fix: initial_peak_guesses refuses unknown models instead of falling back (v0.1.5) - #64
Merged
Merged
Conversation
The unknown-model fallbacks in initial_peak_guesses (npp = 3 and the positional p$i parameter names) were unreachable: _peaks_to_p0 calls _n_peak_params, which throws "Unknown model" before either fallback is used. Automatic guesses genuinely need the model's parameter layout — the parameter count, which slot holds the width, and how a detected FWHM maps onto it — and assuming a 3-parameter amplitude/center/width layout would silently build a wrong-length or wrong-order p0 for a 4-parameter custom model. The function now validates _is_known_model up front and names the five built-in lineshapes in the error, pointing custom-model callers at fit_peaks' explicit p0 instead of a nonexistent kwarg. Both dead branches are gone, and the docstring states the constraint. The equivalent fallbacks in fit_peaks are NOT dead and are left alone: with an explicit p0, fit_peaks never calls _peaks_to_p0, so a custom lineshape fits and reports positional parameter names. The new test covers both halves — the refusal, and the custom-model fit that still works via p0. 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
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.
What
initial_peak_guessescarried two unknown-model fallbacks —npp = ... : 3and positionalp$iparameter names — that could never run._peaks_to_p0calls_n_peak_params, which throws"Unknown model. Provide n_peak_params manually."before either fallback is reached, so a custom lineshape always errored, just with a message naming a keyword that does not exist anywhere in the package.The function now validates
_is_known_modelup front, names the five built-in lineshapes, and points custom-model callers atfit_peaks' explicitp0. Both dead branches are gone and the docstring states the constraint.Failing fast is the right resolution rather than making the fallback work: automatic guesses need the model's parameter layout — the parameter count, which slot holds the width, and how a detected FWHM maps onto it via
width_scale. None of that is inferable from an arbitrary function, and assuming a 3-parameter amplitude/center/width layout would silently build a wrong-length or wrong-orderp0for a 4-parameter custom model.What is deliberately unchanged
The same-looking fallbacks in
fit_peaks(npp,_model_name,param_names) are not dead, and are left alone. With an explicitp0,fit_peaksnever calls_peaks_to_p0, so a custom lineshape fits normally and reports positional parameter names. Verified before touching anything:Applying the same up-front check there would have removed a working feature.
Tests
Extends the
initial_peak_guessestestset with both halves: the refusal (ArgumentError, message mentions "known lineshape") and the custom-model fit that still works throughp0.Full suite: 1940 passed, 0 failed.
Note, not addressed here
_n_peak_params' message still says "Provide n_peak_params manually", but no function in the package takes such a keyword. After this change its only remaining live caller is the TA peak-spec fit infitting.jl:1037, so a custom peak function there is still pointed at a keyword that does not exist. Out of scope for this fix.🤖 Generated with Claude Code