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
DM-32406: Fix lmfit>1.0.2 failures and cleanup dipole code #251
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the one comment I made about the potentially repeated line, everything looks good.
psf=self.diffim.getPsf(), # hereon: kwargs that get passed to genDipoleModel() | ||
psf=self.diffim.getPsf(), # hereon: kwargs that get passed to makeModel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to duplicate this line? It looks like psf=self.diffim.getPsf(),
wouldn't change between these two lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks: it got fixed on the next commit, but I've cleaned up those two.
There was a squashed `maxfev`-related warning; only the kwarg warnings should be squashed here.
maxfev/maxiter both result in a warning and are ignored by lmfit.
This is a "single instance per test" helper class; name it after the class itself for clarity.
While trying to use this, I discovered that verbose/debug are only used in one test, so there's nothing to be gained except confusion by having them in the helper class.
This is where we would substitute a different fitter (e.g. minpack `least_squares`), which might be worth testing.
lmfit>1.0.2 coerces the data and independent variables to float64, but our internal model images are float32, so the default step sizes are too small and the fits fail. Changing our model images to double would be slower and take up more memory; epsfcn>1e-13 seems to be enough to get good fits: step length is proportional to sqrt(epsfcn). If we switch to least_squares, we would instead set `diff_step` here.
1a0c687
to
181e71c
Compare
No description provided.