Skip to content
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

Handling Float Integrals #18434

Merged
merged 9 commits into from
Feb 23, 2020
Merged

Handling Float Integrals #18434

merged 9 commits into from
Feb 23, 2020

Conversation

Smit-create
Copy link
Member

@Smit-create Smit-create commented Jan 22, 2020

Handling Float Integrals

References to other Issues or PRs

Fixes #17119
Fixes #14431

Brief description of what is fixed or changed

Before:

>>> integrate(x**(0.5)*(1+x))
Traceback (most recent call last):
  ValueError: Non-suitable parameters.

>>> integrate((x-t)**(-1/2)*t, (t,0,x))
 Traceback (most recent call last):
  ValueError: Non-suitable parameters.

After fixing:

>>> integrate(x**(0.5)*(1+x))
0.666666666666667*x**1.5 + 0.4*x**2.5

>>> integrate((x-t)**(-1/2)*t, (t,0,x))
1.33333333333333*x**1.5

Other comments

Release Notes

  • integrals
    • Handling Float Integrals

@sympy-bot
Copy link

sympy-bot commented Jan 22, 2020

Hi, I am the SymPy bot (v152). 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.6.

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. -->
Handling Float Integrals
#### 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://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #17119
Fixes #14431

#### Brief description of what is fixed or changed
Before:
```
>>> integrate(x**(0.5)*(1+x))
Traceback (most recent call last):
  ValueError: Non-suitable parameters.

>>> integrate((x-t)**(-1/2)*t, (t,0,x))
 Traceback (most recent call last):
  ValueError: Non-suitable parameters.
```

After fixing:
```
>>> integrate(x**(0.5)*(1+x))
0.666666666666667*x**1.5 + 0.4*x**2.5

>>> integrate((x-t)**(-1/2)*t, (t,0,x))
1.33333333333333*x**1.5
```
#### Other comments


#### 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
  * Handling Float Integrals
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@asmeurer
Copy link
Member

This would almost certainly break existing integrals that work. integrate can't always take an expression with a numeric parameter replaced with a symbolic one and still give a result. Sometimes a more general answer doesn't even exist. It would be better to fix the underlying algorithm, in this case, meijerg, to work with floating point numbers.

@sylee957
Copy link
Member

sylee957 commented Jan 23, 2020

But whatever, it looks like there are only a few test fixes to be done.

If it does not work (because using symbols in some places like exponents make the problem more difficult), then a way of casting float to rationals and casting back can be suggested. (Though I'm not sure that it is totally a good idea.)

@czgdp1807
Copy link
Member

@Smit-create Are you still working on it? Please resolve the conflicts and address the comments.

@Smit-create
Copy link
Member Author

Actually, I tried to fix it but still unable to find a proper fix yet. Would changing some tests work good?

@Smit-create
Copy link
Member Author

Please review.

@czgdp1807
Copy link
Member

LGTM.

@czgdp1807 czgdp1807 removed their request for review February 22, 2020 17:36
@sylee957 sylee957 merged commit b5edde7 into sympy:master Feb 23, 2020
@oscarbenjamin
Copy link
Contributor

This PR made test_issue_15494 become a lot slower. Previously it took 20s but after this PR it takes 160s.

@Smit-create
Copy link
Member Author

I will have a look at this.

@sylee957
Copy link
Member

Actually I think that this changes are starting to fail in master because of timeouts.
I can consider reverting this patch if no idea is coming up from this.

@Smit-create
Copy link
Member Author

If that is the case, then I suggest it to revert. As soon as, I find the other way around, I will try to fix this with as new PR. I do agree with you

@sylee957
Copy link
Member

I didn’t know how much changes were done in master after this, but I see even reverting PR fails some test.

If the problem is about float integrals, I think that it’s better to test out to use nsimplify and cast floats into rationals, and restore them back.

@Smit-create
Copy link
Member Author

If the problem is about float integrals, I think that it’s better to test out to use nsimplify and cast floats into rationals, and restore them back.

I am thinking of a similar way to approach it.

@Smit-create
Copy link
Member Author

diff --git a/sympy/integrals/integrals.py b/sympy/integrals/integrals.py
index 7cde17a290..c4a70638b2 100644
--- a/sympy/integrals/integrals.py
+++ b/sympy/integrals/integrals.py
@@ -28,6 +28,7 @@
 from sympy.series.order import Order
 from sympy.series.formal import FormalPowerSeries
 from sympy.simplify.fu import sincos_to_sum
+from sympy.simplify import nsimplify
 from sympy.utilities.misc import filldedent
 from sympy.utilities.exceptions import SymPyDeprecationWarning

@@ -398,9 +399,10 @@ def doit(self, **hints):


         if self.has(Float):
             fvals = self.atoms(Float)
-            fsubs = {f: Dummy() for f in fvals}
-            rsubs = {s: f for f, s in fsubs.items()}
-            return self.subs(fsubs).doit(**hints).subs(rsubs)
+            fsubs = {f: nsimplify(f) for f in fvals}
+            rresult_num, rresult_den = self.subs(fsubs).doit(**hints).as_numer_denom()
+            return rresult_num.evalf()/rresult_den.evalf()

The above diff leads some small test failures, but makes the test_issue_15494 faster. Can you please help if there is some better way than this to handle this?

@sylee957 sylee957 mentioned this pull request Apr 4, 2020
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.

Integral fails to evaluate integrate((x-t)**(-1/2)*t,(t,0,x)) raises ValueError
6 participants