Skip to content

Conversation

@smichr
Copy link
Member

@smichr smichr commented Jun 25, 2019

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 positive and not extended_positive is 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

  • integrals
    • assumptions based upon definite integration limits are automatically applied to integration variables

@sympy-bot
Copy link

sympy-bot commented Jun 25, 2019

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:

  • integrals
    • assumptions based upon definite integration limits are automatically applied to integration variables (#17093 by @smichr)

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.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->

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 `positive` and not `extended_positive` is 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

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
- integrals
    - assumptions based upon definite integration limits are automatically applied to integration variables
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@smichr
Copy link
Member Author

smichr commented Jun 25, 2019

@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.

@smichr smichr force-pushed the 7334 branch 2 times, most recently from c2731f2 to 87009dc Compare June 25, 2019 09:01
@oscargus
Copy link
Contributor

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...)

@smichr smichr changed the base branch from 7334 to master June 25, 2019 09:02
smichr added 4 commits June 25, 2019 16:56
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
Copy link

codecov bot commented Jun 25, 2019

Codecov Report

Merging #17093 into master will increase coverage by 12.916%.
The diff coverage is 90.909%.

@@              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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integration with Piecewise function returns incorrect answer.

6 participants