-
Notifications
You must be signed in to change notification settings - Fork 8
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
gh-328: efficient resampling in ellipticity_ryden04
#341
Conversation
From the other PR:
Maybe I am misunderstanding, but this wouldn't work even now, would it? In any case, all of the inputs must be broadcastable to a single shape (or |
Right, it won't. Maybe I was just overthinking all the possible edge cases haha, thanks! |
8fd7837
to
01a5587
Compare
01a5587
to
0c2b53d
Compare
Oops, my bad, thanks for the suggestions! |
gh-328: efficient resampling in ellipticity_ryden04 Co-authored-by: Nicolas Tessore <[email protected]>
Co-authored-by: Nicolas Tessore <[email protected]>
68367a3
to
e7e5e9b
Compare
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.
Great! 🚀
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.
LGTM
while np.any(bad): | ||
eps[bad] = rng.normal(mu, sigma, size=size)[bad] | ||
bad = eps > 0 | ||
while np.any(bad := eps > 0): |
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.
Love a good walrus
The code now generates only the required values while resampling making it more efficient. The implementation works with the existing test cases, but given that the function accepts
ArrayLike
args, the following fails -I am not sure if this is a "valid" case or will scientists never pass in such arguments.
Refs: #328
Changed: resampling in
ellipticity_ryden04
is more efficient now