Fix Plot crashing when given a MultiFit inside a list (fixes #246) - #260
Open
cverstege wants to merge 2 commits into
Open
Fix Plot crashing when given a MultiFit inside a list (fixes #246)#260cverstege wants to merge 2 commits into
cverstege wants to merge 2 commits into
Conversation
Plot.__init__ only recognized a MultiFit if it was passed directly; a MultiFit wrapped in a sequence (e.g. Plot([multi_fit])) fell through to the regular fit-object path, where MultiFit.PLOT_ADAPTER_TYPE is None, causing a confusing "'NoneType' object is not callable" crash. This is exactly what the plot() wrapper function does internally, so k2.plot(multi_fit) was broken even though Plot(multi_fit) worked. Plot now unwraps a lone MultiFit found inside a sequence the same way it unwraps one passed directly, and raises a clear NotImplementedError if a MultiFit is mixed with other fits in the same sequence (not yet supported). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Multi-line textwrap.dedent(...) calls and string concatenations that used to need wrapping now fit on one line under the black version pip currently installs, so make lint (and CI's Linting job) flagged them as needing reformatting. No functional changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a regression/edge case where k2.plot(multi_fit) (via the wrapper) would crash when a MultiFit instance was wrapped inside a one-element sequence, by teaching Plot.__init__ to detect and unwrap that case (and to fail fast with a clear error for unsupported mixed lists).
Changes:
- Update
Plot.__init__to unwrap a loneMultiFitfound inside an iterable (e.g.[multi_fit]) and raiseNotImplementedErrorif aMultiFitis mixed with other fits. - Add regression tests covering
Plot(multi_fit),Plot([multi_fit]), thewrapper.plot(multi_fit)path, and the mixed-list error case. - Apply formatting-only updates (Black-driven) to several YAML representer test files and
plot.pystring constants.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| kafe2/fit/_base/plot.py | Unwraps MultiFit when passed inside a one-element sequence; adds explicit error for unsupported mixed sequences; minor formatting changes. |
| kafe2/test/fit/test_plot.py | Adds regression tests ensuring MultiFit plotting works through both Plot and the wrapper plot() helper. |
| kafe2/test/fit/test_representers_parametric_model_yaml.py | Formatting-only updates to multiline YAML string concatenations. |
| kafe2/test/fit/test_representers_model_function_yaml.py | Formatting-only updates to multiline YAML string concatenations (including raw strings). |
| kafe2/test/fit/test_representers_format_yaml.py | Formatting-only updates to multiline YAML string concatenations. |
| kafe2/test/fit/test_representers_fit_yaml.py | Formatting-only updates to multiline YAML string concatenations. |
| kafe2/test/fit/test_representers_container_yaml.py | Formatting-only updates to multiline YAML string concatenations. |
| kafe2/test/fit/test_representers_constraint_yaml.py | Formatting-only updates to multiline YAML string concatenations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
k2.plot(multi_fit)crashed withTypeError: 'NoneType' object is not callable(see wrapped plotting function does not work with multifit #246). Root cause: the
plot()wrapper always wraps its argument in a list beforeconstructing
Plot(...). Since aMultiFitisn't iterable,plot(multi_fit)buildsPlot([multi_fit])— andPlot.__init__only special-cased aMultiFitpassed directly(
isinstance(fit_objects, MultiFit)), so aMultiFithidden inside a list fell throughto the regular per-fit path, where
MultiFit.PLOT_ADAPTER_TYPEisNone, causing the crash.Plot.__init__(kafe2/fit/_base/plot.py) now also unwraps a loneMultiFitfound insidea sequence, the same way it already unwraps one passed directly. So both
Plot(multi_fit)and
Plot([multi_fit])(and thereforek2.plot(multi_fit)) work correctly.MultiFitis mixed with other fits in the same sequence (e.g.Plot([multi_fit, other_fit])) — a case that isn't supported today, sincePlotcan onlytrack a single combined "global" fit-info block — this now raises a clear
NotImplementedErrorinstead of the confusingTypeErrorfrom before.TestMultiFitPlottest class inkafe2/test/fit/test_plot.pycovering all of theabove (direct
MultiFit,MultiFitin a list, theplot()wrapper, and the mixed-listerror case).
blackreformat of a few files(
kafe2/fit/_base/plot.py, severalkafe2/test/fit/test_representers_*_yaml.py) needed toget CI's
Lintingjob green again —black's formatting rules for multi-linetextwrap.dedent(...)calls/string concatenations changed between the version pinned whenthose files were last touched and the version
pip installcurrently resolves (the repodoesn't pin a
blackversion). No functional changes.Test plan
make test(pytest + coverage/unittest run): 850 passed, 80 skipped, 0 failed.make lint(isort, black, flake8): clean.examples/withMPLBACKEND=Agg: all exit 0, including theexamples/011_multifit/*.pyscripts.k2.MultiFit([...]).do_fit()thenk2.plot(mfit)) — no longer crashes.Plot([multi_fit, other_fit])now raises a clearNotImplementedErrorinsteadof a confusing
TypeError.🤖 Generated with Claude Code