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

Make the solver an attribute of the Model class #133

Closed
raoulcollenteur opened this issue Jul 31, 2019 · 10 comments
Closed

Make the solver an attribute of the Model class #133

raoulcollenteur opened this issue Jul 31, 2019 · 10 comments
Assignees
Labels
bug Indicates an unintended behavior or coding error development Indicates development of new features enhancement Indicates improvement of existing features
Milestone

Comments

@raoulcollenteur
Copy link
Member

I would like to store the Solver object as an attribute of the Model class. If something goes wrong now within the call to the solver we can't access the whole object which is annoying for debugging. This will also help in developing new solvers I plan to work on next month.

This would also mean we don't have to initiate a solver object every time we solve. This will only be an internal change for now.
Within the solve method it would look something like this:

self.solver = Solver(args)
self.solver.solve(args) or ml.solver.minimize(args)

and then we can access the solver as:

ml.solver

Anyone against this proposal?

Ps. looks like I'll need to block a day or two to do some programming on Pastas next week so other enhancements I could implement in the solver are welcome. :)

@raoulcollenteur raoulcollenteur added the enhancement Indicates improvement of existing features label Jul 31, 2019
@raoulcollenteur raoulcollenteur added this to the 0.12.0 milestone Jul 31, 2019
@raoulcollenteur raoulcollenteur self-assigned this Jul 31, 2019
@raoulcollenteur
Copy link
Member Author

Possibly add the long standing issue #40 to this.

@raoulcollenteur
Copy link
Member Author

lmfit 0.9.13 seems to cause problems when MinimizerResult.covar does not exist. Needs to be fixed as well.

@dbrakenhoff
Copy link
Member

dbrakenhoff commented Aug 1, 2019

I think this is a good idea!

Semi-related to this: can we add the objective function value to the fit report?

I did this in my WellModel PR because of the earlier issues with optimization, and this makes it easy to compare between two solve results. It's also not weird to provide the value of the objective function in the fit report, I think. And there was a nice space for it next to the type of solver in the fit report.

@raoulcollenteur
Copy link
Member Author

raoulcollenteur commented Aug 1, 2019

I'll start a new branch for these changes in a bit. Let's make a To Do list.

  • Store solver as attribute of model
  • Add solve method to solvers
  • Fix covar bug for LmFit
  • Calculate parameter correlations from covariance matrix
  • Add correlations to fit_report
  • Make all "vary" parameters real booleans
  • ��Move objective function out of solver class
  • Add objective function value top fit_report

EDIT: solver-branch now created to work on the solvers.

@raoulcollenteur raoulcollenteur added bug Indicates an unintended behavior or coding error development Indicates development of new features labels Aug 1, 2019
@mbakker7
Copy link
Collaborator

mbakker7 commented Aug 6, 2019 via email

@dbrakenhoff
Copy link
Member

I was discussing some pastas results with Ruben (specifically the solver quitting after 2 iterations resulting in evp = 0.0 when solving with freq="14D", while solving on a daily basis yields an EVP of 95%) and we figured it would be nice to have an option in the solver to log the parameter values for each iteration.

So for example, if the log_level was set to debug, log the starting values and the subsequent parameter values least squares decides on, and also maybe log some of the output that least squares generates after it's done (e.g. parameters hitting bounds etc.)

@raoulcollenteur
Copy link
Member Author

raoulcollenteur commented Aug 6, 2019

This is already possible by using using the callback argument implemented in the Solver but you'd have to define your own callback method. I used this for the example you mentioned.

@dbrakenhoff
Copy link
Member

Cool, that works.

@raoulcollenteur
Copy link
Member Author

raoulcollenteur commented Aug 14, 2019

Update, I have started to implement the above changes. I think it is already starting to look good. You can now type:

ml.solver.some_attribute

and get some information that is stored in there. Maybe it would be better to use the name ml.fit..?

I have also changed the behaviour that once a certain solver is chosen (e.g., lmfit) this solver is the default for the model. I think this makes sense.

Couple of things to do still. Big thing is how to implement the objective functions but I'll think about it and come up with some proposal.

@raoulcollenteur raoulcollenteur mentioned this issue Aug 19, 2019
@raoulcollenteur
Copy link
Member Author

I'll start a new branch for these changes in a bit. Let's make a To Do list.

  • Store solver as attribute of model
  • Add solve method to solvers
  • Fix covar bug for LmFit
  • Calculate parameter correlations from covariance matrix
  • Add correlations to fit_report
  • Make all "vary" parameters real booleans
  • ��Move objective function out of solver class
  • Add objective function value top fit_report

EDIT: solver-branch now created to work on the solvers.

Bringing the final two changes to another projects in issue #40 .

Just did a PR with the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unintended behavior or coding error development Indicates development of new features enhancement Indicates improvement of existing features
Projects
None yet
Development

No branches or pull requests

3 participants