-
Notifications
You must be signed in to change notification settings - Fork 116
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
lazy evaluation of rational powers #641
base: master
Are you sure you want to change the base?
Conversation
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
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 for the PR! Could you add some tests for ^(a::SN, b)
only without simplify
?
Done! Let me know if there is anything else. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #641 +/- ##
==========================================
+ Coverage 81.86% 82.07% +0.20%
==========================================
Files 16 16
Lines 1908 1919 +11
==========================================
+ Hits 1562 1575 +13
+ Misses 346 344 -2 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Bowen S. Zhu <[email protected]>
Co-authored-by: Bowen S. Zhu <[email protected]>
Rational powers are currently eagerly evaluated, so that, for example,
and so on. While this might be intentional, it often causes precision loss and makes many analytical expressions rather ugly.
This PR adds branches to
basicsymbolic
and^
to prevent evaluation of powers if the base is a literal number and the exponent is a rational. Integer or float exponents still get evaluated.With this PR, we therefore have behavior like
However, there is a small quirk. Currently
(2x) ^ 2
results in(4//1) * x^2
, i.e. the integer prefactor is converted to a rational, due to the behavior of theunstable_pow
function. I am not sure if this is intentional and I did not follow this conversion in the code in this PR. This means with this PR and a rational exponent, we have(2x) ^ (2//1) == 4 * x^(2//1)
. I guess if this PR gets merged, this behavior should be made consistent, one way or another.I am curious if this all sounds like a reasonable change, or if there other reasons why immediate evaluation of all powers is preferable — please let me know what you think!