-
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
Speed up recursions #307
base: development
Are you sure you want to change the base?
Speed up recursions #307
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #307 +/- ##
===============================================
+ Coverage 96.13% 97.00% +0.86%
===============================================
Files 34 36 +2
Lines 2642 3208 +566
===============================================
+ Hits 2540 3112 +572
+ Misses 102 96 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Generally looks good! Just a couple of small changes:
- Need to update labels at the end of the mul/pow recursion (and add test).
bas = nmo.basis.BSplineEval(5)
add = bas*3
add.basis1.basis1.label == add.basis2.label
- pow should have the same check about label as mul
- I think it's worth changing the references to
_shallow_copy
and_set_shallow_copy_temporarily
toself.__class__
or similar, to make it clear that it's class-level behavior we're changing. - I also think you can remove the
temporarily
part of that method: it's a context manager, so temporarily is implied.
While reviewing this PR, I looked into how to use not-quite-global variables, like the new shallow_copy flag. That flag affects the behavior of some functions of all composite classes, so it can't be instance level (i.e., |
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
I think I should have addressed everything, including adding missing tests! add = nmo.basis.BSplineEval(5)*2
# this would have called deep-copy in original implementation but it would copy the parent as well.
# now it prints nothing
out = add.basis1 * 1
# originally would have printed the same as print(add)
print(out._parent) |
In this PR:
__mul__
:basis * 3
will be equivalent tobasis + basis+basis
__rmul__
enabling right multiplicationn * basis
__pow__
by bisect multiply (log2(n) multiplication, less deep copy)Old clone performance
New clone performance
Old Power Performance
New Power Performance