-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
use assumptions for integration variables #17093
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 (v147). 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.5. Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
|
@oscargus , I had made the previous PR online and couldn't update remotely. Here is the same PR with the wester test modified to remove assumptions. Still, reversing the limits makes a big difference on the time it takes to do the integral...that is a different issue, I think. |
c2731f2 to
87009dc
Compare
|
I do think it solves #16014 as well. Just trying the added code manually it seems like it does. The issue was really that the assumptions should not have to be done manually and since you could remove them in the test, it means that it solves it. Time for order is another thing, which indeed is a problem as such, but not really what the issue was about. (I've been thinking about a fix for this recently, but I do not think that the code would have been as simple and clean as your...) |
The test for 15716 is replaced with a MWE that would fail unless the ValueError is caught, but it does not require integration. The integration result is otherwise modified since the real value of x used during integration allows the log to be split so 3 instead of 2 terms are produced.
Codecov Report
@@ Coverage Diff @@
## master #17093 +/- ##
=============================================
+ Coverage 61.56% 74.476% +12.916%
=============================================
Files 623 623
Lines 161197 161219 +22
Branches 37838 37848 +10
=============================================
+ Hits 99234 120071 +20837
+ Misses 56270 35815 -20455
+ Partials 5693 5333 -360 |
References to other Issues or PRs
fixes #7334
Brief description of what is fixed or changed
definite integration limits are used to assign assumptions for the integration variables when they differ from the integration variable.
Other comments
The more strict assumption (e.g. positive when limits are nonnegative) is used since whether 0 is contained at a limit or not will not matter. (see @asmeurer comment on linked issue) Since
positiveand notextended_positiveis used, this will also imply that the variable is finite and, again, the result of the integral should not depend on the inclusion of +/-oo.Release Notes