-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
fix(core): don't use evaluation in assumptions #23693
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
Conversation
✅ Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.13. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
2c0101c
to
e6cfd17
Compare
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[e0d6b5cf] [24141c16]
- 95.0±2ms 62.6±0.8ms 0.66 integrate.TimeIntegrationRisch02.time_doit(10)
- 93.2±2ms 61.7±0.5ms 0.66 integrate.TimeIntegrationRisch02.time_doit_risch(10)
Full benchmark results can be found as artifacts in GitHub Actions |
The change here exposed a bug in |
955d336
to
c42cc25
Compare
Thanks @smichr. I've incorporated your suggestions with small changes. I'm marking this as ready for review. |
sympy/core/add.py
Outdated
elif a.is_Mul and S.ImaginaryUnit in a.args: | ||
coeff, ai = a.as_coeff_mul(S.ImaginaryUnit) | ||
if ai == (S.ImaginaryUnit,) and coeff.is_extended_real: | ||
im_I.append(a*S.ImaginaryUnit) |
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.
This general pattern does still create an expression though, right?
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 does. I want here with @smichr's suggestion that reduces the impact of the change slightly.
Thinking about it now im_I.append(Mul(*ai, evaluate=False))
would be better so that we know that we produce a smaller Mul without depending on evaluation.
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.
Rather im_I.append(-coeff)
. If a = I*coeff
then I*a = -coeff
.
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've just changed this to -coeff
.
The general idea here about removing recursive evaluation in assumptions is good. But can we really rely on assumptions functioning in evaluate=False? Do we know that the basic canonicalization that is disabled by evaluate=False would never be implicitly assumed in an assumption handler? |
No, we don't know that but really that constraint should be respected by the assumptions system. Code in the assumptions system that does anything else should be changed (and there are many examples). |
210e1ad
to
dd0bab7
Compare
This is the one remaining release blocker. Can anyone review it? If not I'll remove the milestone. |
This looks ok to me (though I am sure I could be missing something in the Complements and ConditionSet logic. But I don't see a reason to hold this up. (I just happened to notice this in a look at recently changed PRs so just finished relooking at it.) |
Thanks! |
References to other Issues or PRs
Fixes #22428
See also the discussion about evaluation in the assumptions system here: #23488 (comment)
Brief description of what is fixed or changed
Avoid creating new expressions during assumptions queries. This makes the old assumptions slightly less powerful but at the same time faster and simpler and more robust. This also fixes a bug making it safe to query the old assumptions under
evaluate(False)
.Other comments
Release Notes
evaluate(False)
was fixed.