Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typo in _buildmodel dict restore (Fixes #956) #957

Merged
merged 1 commit into from
May 11, 2024

Conversation

stuart-cls
Copy link
Contributor

Description

Correct typo in _buildmodel dict restore (Fixes #956)

The issue only manifested on Voigt and SkewedVoigt models for me, not sure if you want to change the test_model_saveload.test_save_load_model code just to catch this.

Type of Changes
  • Bug fix
  • New feature
  • Refactoring / maintenance
  • Documentation / examples
Tested on

lmfit: 1.3.1.post0+g3cfd09a.d20240510, scipy: 1.13.0, numpy: 1.26.4, asteval: 0.9.32, uncertainties: 3.1.7

Verification

Have you

  • included docstrings that follow PEP 257?
  • referenced existing Issue and/or provided relevant link to mailing list?
  • verified that existing tests pass locally?
  • verified that the documentation builds locally?
  • squashed/minimized your commits and written descriptive commit messages?
  • added or updated existing tests to cover the changes?
  • updated the documentation and/or added an entry to the release notes (doc/whatsnew.rst)?
  • added an example?

@reneeotten
Copy link
Contributor

thanks for the fix @stuart-cls. It would certainly be good if this was discovered by our test-suite, so I would be in favor of adding/expanding the current tests to cover all built-in models. If you'd be willing to add that to this PR that would be great. If not, that's also fine and I would be okay with merging this PR - we can expand the tests in a follow-up.

@newville
Copy link
Member

@stuart-cls Thanks - I'm a little surprised our existing tests did not catch that. But I agree with @reneeotten that we can make sure this gets tested better.

This is simple / clear enough that we can just merge this. Thanks!

@newville newville merged commit 7710da6 into lmfit:master May 11, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_buildmodel state dict (version 2) restore typo
3 participants