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

MinimizerResult and ModelResult inconsistencies #359

Open
tritemio opened this issue Jul 22, 2016 · 8 comments
Open

MinimizerResult and ModelResult inconsistencies #359

tritemio opened this issue Jul 22, 2016 · 8 comments

Comments

@tritemio
Copy link
Contributor

The two similar classes MinimizerResult and ModelResult inherit from different objects (the former from object the latter from Minimizer).

Also the MinimizerResult doc states:

    Notes
    -----
    Additional attributes not listed above may be present, depending on the
    specific solver. Since this class is essentially a subclass of dict
    with attribute accessors, one can see which attributes are available
    using the `keys()` method.

which is not true ATM.

The two classes should inherit from the same object and possibly from dict.

@newville
Copy link
Member

@tritemio Good point. Off hand, I would think that ModelResult should be a subclass of MinimizerResult. It might be reasonable for MinimizerResult to be a subclass of scipy.optimize.OptimizeResult, which is dict-like.

OTOH, it might be most natural for MinimizerResult to inherit from Minimizer and ModelResult to inherit from Model.

I don't have a strong preference, but it does seem like we could easily break existing scripts and/or expectations with such a change. So, we should be sure to get any changes right the first time, and after some careful consideration. Can we delay until 0.9.6?
Any suggestions for how to handle this?

@tritemio
Copy link
Contributor Author

Sure, this can be postponed to 0.9.6.

It makes sense to make ModelResult inherit from MinimizerResult. In this way we can document only the additional attributes in ModelResult.

A subset of dict-like behaviour, namely attributes access via [], and some iterator method can be desirable but it can be implemented even without inheritance from dict. For specialized classes it is usually cleaner to put a dict in a _data attribute and then implement only the part of the dict API that is of interest. In this way the API is kept small and well-defined and the internal implementation can be easily changed. Instead, when inheriting from dict, we automatically provide the full dict API and the implementation is harder to modify later on.

The dict-like API for these objects can be limited to:

  • attribute access as item ([])
  • .keys() and .items() methods
  • name in object syntax

In the future, these classes can also gain tidying methods.

For now I would simply remove that note in the documentation since all the attributes of MinimizerResult are (or should be) documented and the class does not have a keys() method.

@newville
Copy link
Member

@tritemio OK, thanks for that. I think we agreed to tag current master as 0.9.5 tomorrow and pushing that as a release, basically to fix 0.9.4 for the missing #355 PR, and to officially drop support for Python 2.6.

Is that still OK?

@newville
Copy link
Member

@tritemio (sorry for quick followup).... or should the docs on MinimizerResult be updated for 0.9.5?

@tritemio
Copy link
Contributor Author

@newville, I made a PR removing the note from the docs. Further modifications can go in >0.9.5.

@tritemio
Copy link
Contributor Author

@newville, yes for me it's ok to tag 0.9.5 from current master on Tuesday. Thanks!

@newville
Copy link
Member

@RoyiAvital Use the mailing list for questions about lmfit. Do not hijack unrelated issues.

@angelo-peronio
Copy link

angelo-peronio commented Oct 9, 2020

My personal pet peeve: most of the times I use the Model interface, but I need the Minimizer interface to fit multiple datasets with shared parameters. I am never able to remember that

model_fit_report = model_result.fit_report()

but

minimizer_fit_report = lmfit.fit_report(minimizer_result)

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

3 participants