Require dill #940
Require dill #940
Conversation
…is back compatible
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #940 +/- ##
==========================================
+ Coverage 93.10% 93.11% +0.01%
==========================================
Files 10 10
Lines 3711 3720 +9
==========================================
+ Hits 3455 3464 +9
Misses 256 256 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@newville I am fine with always requiring dill that does make life easier indeed. The change should be documented in doc/installation.rst as well.
One comment on the proposed change for you to consider, other than that I am fine with merging it.
lmfit/model.py
Outdated
| now supports versions of 'state'. | ||
|
|
||
| State Versions: | ||
| '1': state is a tuple of length 9: |
There was a problem hiding this comment.
this is a bit confusing to me as there is actually no version "state 1", at least not explicitly. I assume this state is how things are saved before this PR - correct? I think the description could be a bit more clear on this.
In fact, I am not really sure the additional complication is needed. To me it appears that the "previous" way of doing things would always result in a tuple, the "new" method will save things as a dict. That distinction in itself should be sufficient to distinguish what to do with the input (and ideally after a while, we can just deprecate and eventually remove reading of the tuple state.
The version key seems not really necessary, I would hope that we don't need to change this anymore and we ideally don't want to have to deal with writing/reading of different versions. Are you not sure yet about the structure of this yet; it would be good to make sure to have that all settled before implementing this change.
There was a problem hiding this comment.
@reneeotten Thanks -- will fix
lmfit/model.py
Outdated
| (fname, func, name, prefix, ivars, pnames, | ||
| phints, nan_policy, opts) = left | ||
| elif isinstance(left, dict) and 'version' in left: | ||
| if left['version'] == '2': |
There was a problem hiding this comment.
@reneeotten Thanks - I'll look at this. I think the idea was to attempt to be future-proof.
…-proofing for version check
|
thanks @newville - looks food to me. Perhaps update the "dill" requirement in "doc/installation.rst" and then it should be good to (squash) merge. |
Description
This requires the dill package to be installed for lmfit. That simplifies several tests for dill in the code.
This also includes a change (with back-compatible checks) to the format for a saved Model, with
__setstate__/__getstate__.Also included here are tests of loading models across Python versions -- in general this does not work and model functions need to be re-provided for successfully loading a model saved from a different version. The added tests add appropriate messages and successfully load models when the objective function is provided.
Type of Changes
Tested on
Python: 3.11.5 | packaged by conda-forge | (main, Aug 27 2023, 03:35:23) [Clang 15.0.7 ]
lmfit: 1.2.2.post37+g3e05019, scipy: 1.11.4, numpy: 1.26.2, asteval: 0.9.31, uncertainties: 3.1.7
Verification
Have you