Document that min_nd is unreachable on small data - #244
Open
neuromechanist wants to merge 2 commits into
Open
Conversation
The table gave one default per stop, but max_iter and do_newton differ between backends: AMICA/AMICATorchNG default to max_iter=100 and do_newton=False, while AMICA_NumPy resolves both from the bundled params.json (2000, True). The single-column form also made the section self-contradictory -- at max_iter=100 the min_dll stop it describes as firing at iteration 326-1076 can never fire, so the default PyTorch run is iteration-limited rather than converged. Split the table by entry point and say so. Also name the section as it is actually titled and note that numpy_impl spells min_nd as min_grad_norm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
neuromechanist
added a commit
that referenced
this pull request
Aug 16, 2026
Two review findings, same root cause: the section was written against PR #244's validation.md rewrite, which is not on dev. It cross-referenced a claim ('in all three implementations') that exists only on that branch, so merging in the wrong order would have made the reference false. Dropped; the point is now stated without asserting what another page says. It also framed 'always runs the full budget' as unique to MLX, citing the iteration 326-1076 min_dll measurement. But PyTorch reaches its own max_iter=100 default first, so that contrast was wrong in the same way the validation.md table was before #244 corrected it. The real distinction is what raising max_iter buys: on the other backends a larger budget lets the likelihood stop end the fit, on MLX it is simply spent. Also name numpy_impl's min_grad_norm in the row and the prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
neuromechanist
added a commit
that referenced
this pull request
Aug 16, 2026
* Record the MLX convergence-stop divergence The backend table listed Newton, PDF families, sharing, rejection, precision and rank detection, but not the convergence stops -- and MLX implements neither. It has no min_dll/use_min_dll/maxincs and no min_nd/use_grad_norm/ndtmpsum, so its only outcomes are max_iter, lrate_floor and the degenerate reasons: an MLX fit always runs the full budget while the other backends stop on likelihood stagnation. This is the one MLX limitation that does not fail loudly. Unsupported parameters are absent from the constructor (TypeError) and do_newton / non-GG pdftype raise NotImplementedError, but there is no parameter to reject for a stop that simply does not exist. Also correct the keep_best / MIR / persistence rows, which differ per backend rather than tracking MLX. Refs #248. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Make the MLX stop divergence self-contained and accurate Two review findings, same root cause: the section was written against PR #244's validation.md rewrite, which is not on dev. It cross-referenced a claim ('in all three implementations') that exists only on that branch, so merging in the wrong order would have made the reference false. Dropped; the point is now stated without asserting what another page says. It also framed 'always runs the full budget' as unique to MLX, citing the iteration 326-1076 min_dll measurement. But PyTorch reaches its own max_iter=100 default first, so that contrast was wrong in the same way the validation.md table was before #244 corrected it. The real distinction is what raising max_iter buys: on the other backends a larger budget lets the likelihood stop end the fit, on MLX it is simply spent. Also name numpy_impl's min_grad_norm in the row and the prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Closes #218.
Documentation only; no behavior change.
min_nddefaults to1e-7in Fortran and both Python backends, and is not reachable on a recording the size of the bundled sample in any of the three. Fixing the scale bugs in #212 made the quantity correct without making the stop usable, which is a surprise worth writing down rather than leaving for the next person to rediscover.Added
A "Which convergence criterion actually stops a fit" section in
docs/guides/validation.md, with a table of all four stops and whether each fires on the bundled sample, plus the reference binary's own gradient-norm trace (oscillating 2.5e-5 to 4.7e-5 through iteration 2000, plateauing at 1.0-1.65e-5 out to 5073, never crossing1e-7).It states plainly that this is a property of the data — 30504 samples against 1024 free parameters in
Aalone gives the natural-gradient residual a finite-sample noise floor above the threshold — and that the reference ships the same default and behaves the same way, so retuning it would mean changing a Fortran-faithful default to manufacture an outcome.It also says what has not been determined: the data size at which
min_ndbecomes meaningful is uncharacterized. Rather than leave that implicit, the guidance is to measure the plateau on your own recording and set the threshold above it, or leavemin_dllto do the work (it fires at iteration 326-1076 depending on the BLAS build, per #213).Cross-references
The
min_nddocstring intorch_impl/core.pyand themin_grad_normdefault innumpy_impl/core.pyboth now point at that section, so someone reading the parameter learns it before running a fit that never stops on it.