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

subs cannot target RootOf args #10097

Merged
merged 2 commits into from
Nov 10, 2015
Merged

subs cannot target RootOf args #10097

merged 2 commits into from
Nov 10, 2015

Conversation

smichr
Copy link
Member

@smichr smichr commented Nov 1, 2015

TODO

  • check that r = RootOf(..); r.subs(r, foo) still works

closes #10092

RootOf should be treated like a literal and should not allow subs to target any of its args. Even a change of variables is meaningless since RootOf caches the expression used at instantiation:

in master:

>>> RootOf(x**3-2*x**2+x-1,0)
RootOf(x**3 - 2*x**2 + x - 1, 0)
>>> RootOf((x**3-2*x**2+x-1).subs(x,var('y')),0)
RootOf(x**3 - 2*x**2 + x - 1, 0)

It would be possible to allow targetting of the root number with an integer (but if the integer is wrong, an error will be raised). Please feel free to make suggestions.

I don't know why refine is being used on the test in 8545. There is
nothing in the issue about it and it takes a long time to do nothing.

>>> var('x')
x
>>> eq = 1 - x - sqrt((1 - x)**2);eq
-x - sqrt((-x + 1)**2) + 1
>>> refine(eq)
-x - sqrt((-x + 1)**2) + 1
@smichr
Copy link
Member Author

smichr commented Nov 4, 2015

I suppose one could allow the integer to be replaced with another integer and then let any error raise on their own if the integer is out of range. Any thoughts? Should I just commit this as is and let the user-base indicate via issue that the integer substitution (e.g. RootOf(eq,1).subs(1,2)) is a desired feature? ping @asmeurer

@asmeurer
Copy link
Member

I don't like substituting integers (it would be confusing to change only the second argument and not instances of that integer in the expression).

On the other hand, why shouldn't RootOf((x**3-2*x**2+x-1), 0).subs(x, x + 1) work?

@smichr
Copy link
Member Author

smichr commented Nov 10, 2015

On the other hand, why shouldn't RootOf(x3-2*x2+x-1, 0).subs(x, x + 1) work

x in the RootOf expression is really like a dummy variable that retains the name of whatever was used to instantiate the particular RootOf object. If you were to type RootOf(x**3-2*x**2+x-1,0) and then type the same thing but using y instead of x you would get the original object with the x.

If we did allow such replacements, where do we draw the line? Should subs(x**2, x**4) work? If the replacement leads to a RootOf error should it be disallowed (returning the original expression unchanged) or should the error be raised?

For reference, the replacement of x with a RootOf object is the source of the issue that motivated this PR. It was an expression like (x < RootOf(...)).subs(x, RootOf(...)) where the desired result was False.

@asmeurer
Copy link
Member

I guess it makes sense. The x in the RootOf is not free, so it shouldn't be expected to work with subs. There's still a lot of inconsistency here among the SymPy objects with dummy variables, but I agree now that this is the right fix.

asmeurer added a commit that referenced this pull request Nov 10, 2015
subs cannot target RootOf args
@asmeurer asmeurer merged commit f876779 into sympy:master Nov 10, 2015
@smichr smichr deleted the 10092 branch September 12, 2018 17:48
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.

subs into inequality involving RootOf raises GeneratorsNeeded
2 participants