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

Cannot deserialize ModelResult saved before 1.1.0 #845

Closed
jcjaskula-aws opened this issue Mar 3, 2023 · 3 comments
Closed

Cannot deserialize ModelResult saved before 1.1.0 #845

jcjaskula-aws opened this issue Mar 3, 2023 · 3 comments

Comments

@jcjaskula-aws
Copy link
Contributor

jcjaskula-aws commented Mar 3, 2023

First Time Issue Code

Yes, I read the instructions and I am sure this is a GitHub Issue.

Description

A ModelResult loaded from a file saved with a version older than 1.1.0 will error when users try to print an HTML report. This is due to the fact that old serialized models do not contain the rsquared attribute, which is then set to None at loading. Printing an HTML report calls gformat (that accepts only float) on result.rsquared will result in a crash.

More precisely, rsquared is set to None by default here but its type is not checked there.

A Minimal, Complete, and Verifiable example

https://gist.github.com/jcjaskula-aws/81f78f5b712b5a87d574bb7dd3cc46f6

Error message:
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.10/site-packages/lmfit/model.py", line 1651, in _repr_html_
    report = fitreport_html_table(self, show_correl=show_correl,
  File "/usr/lib/python3.10/site-packages/lmfit/printfuncs.py", line 249, in fitreport_html_table
    stat_row('R-squared', gformat(result.rsquared))
  File "/usr/lib/python3.10/site-packages/lmfit/printfuncs.py", line 62, in gformat
    expon = int(log10(abs(val)))
TypeError: bad operand type for abs(): 'NoneType'
Version information

Python: 3.10.10 (main, Feb 8 2023, 05:34:50) [Clang 14.0.0 (clang-1400.0.29.202)]

lmfit: 1.1.0, scipy: 1.9.3, numpy: 1.23.5,asteval: 0.9.29, uncertainties: 3.1.6

@newville
Copy link
Member

newville commented Mar 3, 2023

@jcjaskula-aws OK, yes, we should check for a valid value of rsquared there. I'll fold that into #844

@jcjaskula-aws
Copy link
Contributor Author

Sounds great. Just in case 1.2.0 would have breaking changes, it might be interesting for me to commit this modification to the current main and tag it 1.1.1.

@newville
Copy link
Member

newville commented Mar 3, 2023

1.2 will not have intentional breaking changes, and I believe no significant modifications to existing tests. It will add new and enhanced features, which makes it more than only bug fixes.

One thing we are not testing is reading models saved with older versions. I think we should add tests for that, exactly to avoid this sort of problem in the future.

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

No branches or pull requests

2 participants