-
Notifications
You must be signed in to change notification settings - Fork 107
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
Remove implicit scipy dependency. #1305
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.
A couple comments/questions, but this is essentially fine.
lambda_ *= 3 # increase damping | ||
|
||
if np.linalg.norm(delta_params) < tol: | ||
break |
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.
I assume you copied this algorithm from somewhere. Should give the source.
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.
It's ChatGPT + wikipedia. I can add a comment to that effect.
tests/test_vonkarman.py
Outdated
lams = [300.0, 500.0, 1100.0] | ||
r0_500s = [0.05, 0.15, 0.3] | ||
L0s = [1e10, 25.0, 10.0] | ||
do_deltas = [False, True] |
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.
How slow were the slow tests? I'm assuming you checked this and found the "slow" ones were still fast enough to run all the time.
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.
On my laptop this this is 2.07 sec for all tests vs 0.07 sec for just the fast tests. I can wrap this in a if __name__ == '__main__'
block if you prefer.
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.
Sure. I usually try to keep each test under 1 sec if possible for they pytest run. There are some exceptions, but this seems like one where we don't need the slow tests to spot regressions.
galsim/utilities.py
Outdated
params = np.array(x0) | ||
lambda_ = lambda_init | ||
|
||
for _ in range(max_iter): |
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.
The code coverage miss is this loop never ending. Should either add a test with a low max_iter, (e.g. 1 probably), or add #pragma: no branch
to this line.
Addresses #1253 .
Adds a basic Levenberg-Marquardt optimizer that can fill in for
scipy.linalg.least_squares
in simple cases (such as used inFittedSIPWCS
). It's a small bit slower on average, so we might want to consider making it a fallback option when scipy is unavailable though I didn't bother for now. (The fitted sip unit tests takes 3.0 s now, vs. 2.25 s using scipy).Along the way I also refactored our
if __name__ == __main__
test running code so you can use-k testname
(similar to pytest) when running directly withpython
. There are command line options for running the profiler too.