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

Fixing integral subs #2390

Closed
wants to merge 3 commits into from
Closed

Fixing integral subs #2390

wants to merge 3 commits into from

Conversation

MechCoder
Copy link
Contributor

First attempt at fixing integral subs.

  1. If old, new in eq.subs(old, new) are both functions of the integration variables, then substitution takes place irrespective of whether eq is a definite or indefinite integral.
  2. If it is a indefinite integral, then this is followed, https://code.google.com/p/sympy/issues/detail?id=3829
  3. If it is definite, and there is a linear transformation, then it is xreplaced.

@jrioux
Copy link
Member

jrioux commented Aug 17, 2013

SymPy Bot Summary: 🔴 Failed after merging Manoj-Kumar-S/Integralfix (aaa1eab) into master (90fc660).
@manoj-kumar-s: Please fix the test failures.
🔴 PyPy 2.0.0-beta-1; 2.7.3-final-42: fail
🔴 Python 2.7.2-final-0: fail
🔴 Python 3.2.1-final-0: fail
Sphinx 1.1.3: pass
Doc Coverage: unchanged
35.931% of functions have doctests (compared to 35.931% in master)
41.310% of functions are imported into Sphinx (compared to 41.310% in master)

@MechCoder
Copy link
Contributor Author

@asmeurer : Could you review this? There are a lot of test failures.

@asmeurer
Copy link
Member

First, please add tests for the new behavior, so that I can easily see what it does and verify that it is correct.

@MechCoder
Copy link
Contributor Author

@asmeurer : I added tests. Could you have a look now?

assert eq.subs(y, x) == Integral(f(y), (y, x))
eq = Integral(f(x), (y, 1, 2))
assert eq.subs(y, x) == Integral(f(x), (x, 1, 2))
expr = Integral(x**log(x)*exp(x))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include the integration variable.

@asmeurer
Copy link
Member

Add tests for substituting against integrals with only the upper limit.

@asmeurer
Copy link
Member

I think it would be better to do the substitution so that it results in a definite integral again if possible. So for example Integral(f(x), x).subs(x, y) should give Integral(f(y), y).

@MechCoder
Copy link
Contributor Author

@asmeurer : And that would be possible only if the transformation is linear right?

@jrioux
Copy link
Member

jrioux commented Aug 20, 2013

I disagree with most of the tests here. The only test that makes sense to me is

expr = Integral(x**log(x)*exp(x)) 
assert expr.subs(x, y) == Integral(x**log(x)*exp(x), (x, y))

.subs has a mathematical meaning whereas the type of replacement implemented in this PR (besides the above) should IMHO be the function of .replace or .xreplace (or yet a fourth function). See the discussion at #2198, especially about @asmeurer's comment about the 1, 2, 3's of subs scope.

f = Function("f")
g = Function("g")
eq = Integral(f(y), y)
assert eq.subs(f(y), g(y)) == Integral(g(y), y)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "eq.subs" is the function that's supposed to do this. My under is that subs is supposed to preserve mathematical equality, and this doesn't.

@asmeurer
Copy link
Member

@manoj-kumar-s I think it would be fine if it only worked for subs(x, y) where x is one of the integration variables and y is a Symbol.

@asmeurer
Copy link
Member

You should also add tests for multiple integrals, especially multiple integrals of varying types (definite and indefinite). Let's also make sure that it works correctly for things like Integral(f(x), (x, 0, x)) where the integration variable is both bound and free.

@asmeurer
Copy link
Member

@jrioux @zanzibar7 thanks for helping review this. @raoulb we discussed this issue a few times on IRC. Do you want to help out here as well?

@MechCoder
Copy link
Contributor Author

@jrioux , @zanzibar7 , @asmeurer : I'm still a bit confused. So do you mean to say only this should work?

`eq = Integral(f(x), x).subs(x, y)` should give `eq = Integral(f(x)., (x, y))`

and what to do abput stuff like eq = Integral(f(x), x).subs(f(x), g(x)) , it should just return a ValueError?

and for a definite integral say eq = Integral(f(x), (x, 1, 2)).subs(x, y) what should it return? and for Integral(f(x), (x, 0, x)).subs(x, y) it should return Integral(f(x), (x, 0, y)). I'm sorry but I read through the discussion on that PR and it has got me slighly more confused.

@MechCoder
Copy link
Contributor Author

So that means bascially subs should work for a definite integral only when the old in subs is an integration variable and the new in subs can be anything? And for indefinite integrals it should raise a ValueError?

@MechCoder
Copy link
Contributor Author

Okay, I'm preparing a gist of rules that have to be followed, so that I can become clear.

@MechCoder
Copy link
Contributor Author

@jrioux , @zanzibar7 , @asmeurer : Could you please have a look at this, and say if my interpretation is right? https://gist.github.com/Manoj-Kumar-S/6277258

@jrioux
Copy link
Member

jrioux commented Aug 20, 2013

I disagree with your point 1: this subs should return Integral(f(x), (x, y)); the operation Integral(f(x), x) -> Integral(f(y), y) should be done by some integral transformation method (which probably already exists but I don't know its name), but not by .subs, IMHO.

I agree with point 2, which is what issue 3829 is about.

I disagree with your point 3 and all its subpoints. Basically, anything that's bound inside Integral isn't free for substitution with .subs (but other method such as .replace and .xreplace might accomplish this). When a .subs finds nothing to substitute, it returns the original expression unchanged and does not raise an exception.

I think you should concentrate on issue 3829 and nothing more, and maybe even hold off a bit, as #2198 looks in good shape to fix up the rest.

@MechCoder
Copy link
Contributor Author

Okay, great.

@jrioux
Copy link
Member

jrioux commented Aug 20, 2013

SymPy Bot Summary: 🔴 Failed after merging Manoj-Kumar-S/Integralfix (95bf661) into master (905dd3e).
@manoj-kumar-s: Please fix the test failures.
🔴 PyPy 2.0.0-beta-1; 2.7.3-final-42: fail
🔴 Python 2.7.2-final-0: fail
🔴 Python 3.2.1-final-0: fail
Sphinx 1.1.3: pass
Doc Coverage: unchanged
35.931% of functions have doctests (compared to 35.931% in master)
41.310% of functions are imported into Sphinx (compared to 41.310% in master)

@asmeurer
Copy link
Member

But isn't Integral(f(x), (x, y)) essentially equivalent to Integral(f(y), y) when y is a Symbol?

@asmeurer
Copy link
Member

Manoj, maybe move that to the wiki, so that we can edit it.

@jrioux
Copy link
Member

jrioux commented Aug 21, 2013

Logic for things like Integral(f(x), (x, y)) replaced by Integral(f(y), y) should go in the class constructor, IMHO (and better make sure to check the assumptions on x and y).

@asmeurer
Copy link
Member

asmeurer commented Oct 5, 2013

SymPy Bot Summary: 🔴 Failed after merging Manoj-Kumar-S/Integralfix (95bf661) into master (5e2ea80).
@manoj-kumar-s: Please fix the test failures.
🔴 Python 2.6.8-final-0: fail
🔴 Python 2.7.3-final-0: fail
🔴 PyPy 1.8.0-final-0; 2.7.2-final-42: fail
🔴 Python 3.2.3-final-0: fail
🔴 Sphinx 1.1.3: fail

@MechCoder
Copy link
Contributor Author

@asmeurer Is anyone working on this right now, in a simultaneous PR. I don't think I have the bandwidth to work on this right now. So closing.

@MechCoder MechCoder closed this Oct 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants