-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Add simplify_real
method to symbolic expressions
#14630
Comments
Author: Michael Orlitzky |
comment:1
MathJAX is throwing a "Math Processing Error" -- I'm not sure whether it's busted or I screwed up but the docs definitely need a review. |
comment:2
You need to do I'm not sure about all the stuff in the doc and the code, but I think that some of the doc is doing the code and vice versa? For instance,
but you don't do that in the code. And the corresponding part in the doc seems to imply that in order to use My question is how this will interact with the other simplifications. I seem to recall that in some of the more controversial (e.g. |
Attachment: sage-trac_14630.patch.gz Add simplify_real() method to Expression |
comment:3
Replying to @kcrisman:
Fixed in the new patch, but my MathJAX still doesn't work, so please give it a look.
Whoops, I forgot to call
I didn't mean that the user has to
Expressions should be complex everywhere. I believe there are one or two functions which treat them as real, but they're documented to do that, so it's fine. Radcan via Not many expressions are actually affected by the domain and 'real' assumptions. We can get most of the benefit of |
comment:4
What about adding a parameter to all the simplify_* functions instead of adding more functions? |
comment:5
Replying to @sagetrac-sluther:
If instead of Particularly with |
comment:6
Well I'd say this feels like mixing different concepts. The log, radical, etc. are about the form of the expression that's going to be transformed, whereas the domain is about the values the variables in the expression may take. You're right that sometimes the application of simplifications in different order yields different results. But I find this rather annoying (not that I know how to fix it). By adding even more functions to the mix we make this situation even worse. I usually use simplify_full(). So as I understand it I'd always need to call simplify_full() and simplify_real() in some order to benefit from the knowledge about the domain. And then I'm still left wondering if there are simplifications in simplify_full() hat could benefit from this knowledge too, but didn't get it because there's only simplify_real() and not simplify_full_real(). Is there a list of which simplify_* function could use this domain parameter? |
comment:7
Instead of adding a new function Example use would be:
|
comment:8
Replying to @sagetrac-sluther: All of the The |
comment:9
Replying to @burcin:
This is way better than what we currently have to do:
but still does two things undesirably:
|
comment:11
Replying to @burcin:
You'd need to document that the user of the context manager should make sure to not relinquish control inside the context manager. Thanks to "yield" in python, lexical enclosure doesn't necessarily mean runtime enclosure. |
comment:15
I think the form/domain-simplify design issue has not been resolved and should be discussed/decided first: the/an author may benefit from presenting a solution to sage-devel, and if the crowd has no opinion then it should be implemented. Thus, I'm setting to needs_info. |
Commit: |
New commits:
|
Branch: u/mjo/ticket/14630 |
comment:18
This seems fine, given that it is extremely unlikely a context manager will appear and that having many options seems undesirable. However, if someone gets a sage-devel discussion on this going I'm not going to stop them.
I was pleased this worked, wasn't sure how much it would do. Maybe the following would be a useful test to show what it does and doesn't do.
Here's another one that, again, pleasantly works as one would think.
Now if only we had to use this for the following simplification!
|
comment:19
Also note this probably conflicts slightly with #11912. |
comment:20
Just making a note to myself here to update all of the "see also" references in the other |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
|
comment:22
I just force-pushed a branch that's rebased on top of #11912 because of a conflict. That one has a positive review, so I guess when it's merged I could rebase this on top of the develop branch? Not sure what's easiest for the release manager. |
Dependencies: #11912 |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:24
And per my note, I've included another left-out function in the |
comment:25
I think it is fine now and certainly should be included, test pass in |
Reviewer: Karl-Dieter Crisman, Ralf Stephan |
Changed branch from u/mjo/ticket/14630 to |
Symbolic expressions in sage are by default assumed complex. There is a maxima variable, called the "simplification domain," which affects whether or not it simplifies
sqrt(x^2)
toabs(x)
. Since our expressions are complex, we set the simplification domain to complex, but provide no easy way to change it.By adding a
simplify_real()
method to Expression, we give the user a way to perform the aforementioned simplification by declaring his expression real.This might provide a quick fix for #14305. See also:
https://groups.google.com/forum/?fromgroups=#!topic/sage-support/jhCJujRtNA4/discussion
Depends on #11912
CC: @egourgoulhon
Component: symbolics
Author: Michael Orlitzky
Branch/Commit:
649e3b3
Reviewer: Karl-Dieter Crisman, Ralf Stephan
Issue created by migration from https://trac.sagemath.org/ticket/14630
The text was updated successfully, but these errors were encountered: