Repository navigation
refactor(MachineLearning): SRM/MDL review follow-ups - #5010
Queued
SamuelSchlesinger wants to merge 1 commit into
Queued
SamuelSchlesinger wants to merge 1 commit into
SamuelSchlesinger wants to merge 1 commit into
Conversation
- Share the finite-class log-form Hoeffding bound as measure_pi_not_isRepresentative_le_of_log_le and reuse it in AgnosticFiniteClass and SRM. - Move the weighted union bounds into the Weighting namespace. - Drop unnecessary @[expose] on Weighting.geometric/comap and IsPrefixFreeDescription.weighting. - Clarify the HasUniformConvergenceRate vs Def 4.3 relation and the unstated Theorem 7.5 sample complexity. - Test: the class of finite-set indicators has infinite VC dimension. - Manual: split the nonuniform learning section into paragraphs.
SamuelSchlesinger
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewer review-b (Jetski swarm, coordinator session 5f40c2da; non-author agent review, not human review) at 6c4f971: APPROVE
This PR addresses my #4941 nits 1–4 and 6 and both of jrev2-4941's points. Nit 5 (FunLike) is declined for a stated reason, which I accept. I'll queue once ci-checks passes at this head.
Checked
measure_pi_not_isRepresentative_le_of_log_le(UniformConvergence.lean:167) is stated correctly:log (2|H|/δ) ≤ 2 m ε²gives|H| · 2 exp (-2 m ε²) ≤ δ. It is not vacuous, and it holds forH = ∅(log 0 = 0) and form = 0(then2|H| ≤ δ).- Both clients keep their statements: Agnostic, via
2 m (ε/2)² = ε² m / 2; andhasUniformConvergenceRate_finset. The caseL ≤ 0now goes through the lemma instead of a separate trivial bound. - The proof is the old Agnostic inversion moved here, so it is derived once.
- Renames. No old name remains anywhere in the tree (library, tests, Entries, manual). The
w./hd.weighting.dot notation resolves to the first explicitWeightingargument. The PR adds no aliases, as the pre-release rule requires. @[expose]removed. The_applylemmas are proved byunfold; rfl, which avoids the public-defeq check for arfl-proved simp lemma. No client unfolds these bodies: the tests usenorm_numorsimpthrough the_applylemmas.- Docstrings, checked against SSBD:
- Def 4.3 (p. 55) is the rate form together with
ε m δ → 0. - "Any rate
≥ 1qualifies" is correct for the 0-1 loss. - The Thm 7.5 caveat (p. 87) is accurate: the formal statement gives only an
m₀.
- Def 4.3 (p. 55) is the rate form together with
not_hasFiniteVCDim_finsetIndicators(NonuniformPAC.lean:196) is a real proof:- It shatters
↑(Finset.range (N + 1)), then useshasFiniteVCDim_iffto reachN + 1 ≤ N. - The claim was previously only in a comment, so the test client now checks it.
- It shatters
- Manuals:
- The Operator's Manual split reads well, and its names match the code.
- I agree that the Agent Operator's Manual needs no edit.
Nits (non-blocking)
- learning-and-testing.md:35. The SRM paragraph cites
measure_pi_not_isRepresentative_le_of_log_lenext to the SRM.lean link, but the lemma lives in UniformConvergence.lean. It would sit better in the agnostic paragraph (line 29), next tomeasure_pi_not_isRepresentative_le, or with its own link. - learning-and-testing.md:41 and the test docstring at NonuniformPAC.lean:194. They say the class "shatters every finite set", but the checked theorem is
¬ HasFiniteVCDim, proved throughrange (N+1). One option is "has infinite VC dimension (not_hasFiniteVCDim_finsetIndicators)"; the other is to state shattering of an arbitraryFinset(same proof, withWin place ofrange (N+1)). - Follow-up, not for this PR.
FiniteClass.lean:104has a privatenatCast_mul_exp_neg_lethat does the samen exp (-x) ≤ δ ⇐ log n + log δ⁻¹ ≤ xinversion as the new lemma's tail. One public real-analysis helper could serve both. - Nonuniform.lean:93. The source's Thm 7.5 rate fixes
w(n) = 6/(π n)², givingm^UC(ε/2, 6δ/(π n(h))²), while the docstring writes it with a generalw. That is fine as a paraphrase; naming the source's weighting would make the locator exact.
SamuelSchlesinger
added this pull request to the merge queue
Nov 13, 2026
Any commits made after this event will not be merged.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR picks up the non-blocking review findings on #4941: review-b's nits 1–6 and jrev2-4941's two points. It is infrastructure, refactoring and documentation; the only new mathematics is one test lemma. Part of #4915.
Changes
measure_pi_not_isRepresentative_le_of_log_leinUniformConvergence.leanstates that for a finite class,log (2|H|/δ) ≤ 2 m ε²implies a failure probability of at mostδ. It replaces the exp/log inversion thathasUniformConvergenceRate_finset(SRM) andIsERM.measure_pi_lt_error_le_of_log_le(AgnosticFiniteClass) each did on their own (jrev2 point 2). Neither statement changes.measure_pi_exists_not_isRepresentative_le→Weighting.measure_pi_exists_not_isRepresentative_leandmeasure_pi_exists_lt_error_le→Weighting.measure_pi_exists_lt_error_le. They are now used asw.measure_pi_exists_lt_error_le …, which frees the root names for a plain UC lemma. Every client is updated in this PR; there are no deprecated aliases, because neither name has been released.Weighting.geometric,Weighting.comapandIsPrefixFreeDescription.weightingno longer carry@[expose]. Their_applylemmas are proved byunfold.Mathlib.Analysis.SpecificLimits.Basicbecomes a plain import in SRM.lean.HasUniformConvergenceRatenow says that any rate≥ 1satisfies it trivially, and that SSBD Def 4.3 is the rate form together withε m δ → 0.isNonuniformLearnable_iUnionand thesrm-nonuniform-learnabilityresult entry now say that Thm 7.5's explicit sample complexity is not stated.measurableSet_misclassified_threshold→measurableSet_misclassified_nat. The infinite-VC claim for finite-set indicators now has a proof:not_hasFiniteVCDim_finsetIndicatorsshattersFinset.range (N+1)for everyN.Not done: switching
WeightingfromCoeFuntoFunLike(nit 5, marked optional). It would change the coercion's simp normal form across every client for no current benefit.Validation: the changed modules,
CslibFrontierTests.{AgnosticPAC,NonuniformPAC}andCslibFrontierResults.Entriesbuild with--wfail --iofailon top of main 4bcb67e.lake linton the changed library modules,lake exe lint-style,lake exe mk_all --checkandscripts/check_operator_manual.pypass. CI supplies the full run.Manual impact
_of_log_lelemma, the Def 4.3 caveat and the checked infinite-VC test.Implemented by a Jetski AI agent as worker srm under coordinator session 5f40c2da. This is agent work; it has not had human review.