Opened 11 years ago
Closed 8 years ago
#11912 closed enhancement (fixed)
Clarify simplify_radical and Maxima's radcan
Reported by: | kcrisman | Owned by: | mvngu |
---|---|---|---|
Priority: | minor | Milestone: | sage-6.4 |
Component: | documentation | Keywords: | sd48 |
Cc: | burcin, zimmerma | Merged in: | |
Authors: | Michael Orlitzky | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | 5d24f4c (Commits, GitHub, GitLab) | Commit: | 5d24f4c71b07beef87cc6b27df7f7cc37c29071e |
Dependencies: | Stopgaps: |
Description (last modified by )
We use Maxima's radcan (warning - link may change) for simplify_radical
. The documentation claims
Simplifies expr, which can contain logs, exponentials, and radicals, by converting it into a form which is canonical over a large class of expressions and a given ordering of variables; that is, all functionally equivalent forms are mapped into a unique form. For a somewhat larger class of expressions, radcan produces a regular form. Two equivalent expressions in this class do not necessarily have the same appearance, but their difference can be simplified by radcan to zero. For some expressions radcan is quite time consuming. This is the cost of exploring certain relationships among the components of the expression for simplifications based on factoring and partial-fraction expansions of exponents.
but it can be really hard to tell exactly what this all means. See this ask.sagemath.org question and #8497, to which this is a followup.
The plan is to rename simplify_radical()
to radcan()
to match the upstream name. We can then alias simplify_radical()
to radcan()
, and deprecate the simplify_radical()
name.
Afterwards we can attempt to clarify the docs, and provide more examples of radcan()
's usage. We should provide both cautionary examples from our tickets, and some of the use cases that Dr. Fateman has described.
Attachments (1)
Change History (31)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
- Description modified (diff)
comment:3 in reply to: ↑ description Changed 9 years ago by
Replying to kcrisman:
The plan is to rename
simplify_radical()
toradcan()
to match the upstream name. We can then aliassimplify_radical()
toradcan()
, and deprecate thesimplify_radical()
name.
Why would we use the Maxima name? It doesn't match any Sage convention. I suggest radical_canonicalize()
or canonicalize_radical()
in anticipation of other canonicalize_*()
methods.
comment:4 Changed 9 years ago by
There is Expression.rectform()
(also my fault), but I don't particularly care about the name. Of those two I think I prefer radical_canonicalize()
.
comment:5 Changed 9 years ago by
Actually, I take that back -- canonicalize_radical()
would be better for tab completion if we add other canonicalize_foo()
methods.
Changed 9 years ago by
comment:6 Changed 9 years ago by
Here's the deprecation/rename, and a few changes to the docs. I haven't been able to come up with any good use cases for radcan(), but I'm not feeling very imaginative today.
comment:7 Changed 9 years ago by
Note: I haven't updated the function name anywhere, so a ton of tests fail. Just looking for some ideas for now.
comment:8 Changed 9 years ago by
- Keywords sd48 added
Yeah, and we'll need a doctest for the deprecation as well. Let's think about this for a day or two, but resolve it by the end of SD 48. I think that looking at the simplifications that simplify_radical()
does would be sufficient.
comment:9 Changed 9 years ago by
See this comment for both some good use cases *and* some potential new aliases...
comment:10 Changed 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:11 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:12 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:13 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:14 Changed 8 years ago by
- Branch set to u/mjo/ticket/11912
- Commit set to f34ddfc2e47508ebc2323e347739f18000c15717
- Status changed from new to needs_review
Rewrote the patch as a git branch, and added the deprecation tests. Also fixed the usage in the rest of the sage library.
New commits:
f34ddfc | Trac #11912: Rename and deprecate Expression.simplify_radical().
|
comment:15 Changed 8 years ago by
Probably see also #3520.
comment:16 Changed 8 years ago by
Any thoughts on canonicalize_exp
or canonicalize_log
? (One could even deprecate the other functions in favor of the other names... just thinking out loud.) I also think this one should depend on #14630, because then you can also mention it in a few examples where it would be relevant (e.g. If you don't want this kind of "simplification,"...
), not to mention the likely conflict in the patches anyway.
comment:17 Changed 8 years ago by
- Commit changed from f34ddfc2e47508ebc2323e347739f18000c15717 to 2d50b886561b9cc98585749b6292eaba269da73c
Branch pushed to git repo; I updated commit sha1. New commits:
2d50b88 | Trac #11912, Trac #3520: Add numerical integral example.
|
comment:18 follow-up: ↓ 19 Changed 8 years ago by
I just pushed another commit adding the example from #3520. I'm against adding canonicalize_exp
and canonicalize_log
methods, since they don't seem to serve a purpose and can cause bugs. For example, see the change I made in relation.py:
#Try to apply some simplifications to see if left - right == 0 - simp_list = [difference.simplify_log, difference.simplify_rational, difference.simplify_exp,difference.simplify_radical,difference.simplify_trig] + simp_list = [difference.simplify_log, difference.simplify_rational,difference.canonicalize_radical,difference.simplify_trig]
Both simplify_exp
and simplify_radical
were being called, wasting cycles, because they're the same thing!
I can see the benefit for things like full_simplify()
where we're mimicking the Mathematica name so that new users can find it through tab completion. But for canonicalize_*
I don't think it's going to do us any good.
I agree that ideally this and #14630 should have some kind of dependency, but practically, they both may sit around waiting for review for a while. I'd rather not put up roadblocks to either one getting reviewed; when one of them gets in, I'll go update the other branch.
comment:19 in reply to: ↑ 18 ; follow-up: ↓ 22 Changed 8 years ago by
I can see the benefit for things like
full_simplify()
where we're mimicking the Mathematica name so that new users can find it through tab completion. But forcanonicalize_*
I don't think it's going to do us any good.
Hmm. So should this even be called canonicalize_radical
or maybe something else that indicates it doesn't just do radicals? I wonder why they chose that name in Maxima - maybe there is something in the algorithm that is 'radical'? (Pun intended.)
comment:20 Changed 8 years ago by
Also, I think you repeated some wording in the doc - maybe it's repeated in Maxima, but still that seems unnecessary here.
+ All functionally equivalent forms are mapped into a unique + form. For a somewhat larger class of expressions, produces + a regular form. Two equivalent expressions in this class + do not necessarily have the same appearance, but their + difference can be transformed by radcan to zero. +
comment:21 Changed 8 years ago by
- Commit changed from 2d50b886561b9cc98585749b6292eaba269da73c to e5ab33994355520f0f27914f73f1680292eadc3d
Branch pushed to git repo; I updated commit sha1. New commits:
e5ab339 | Trac #11912 (review): Remove superfluous documentation paragraph.
|
comment:22 in reply to: ↑ 19 Changed 8 years ago by
Replying to kcrisman:
Hmm. So should this even be called
canonicalize_radical
or maybe something else that indicates it doesn't just do radicals? I wonder why they chose that name in Maxima - maybe there is something in the algorithm that is 'radical'? (Pun intended.)
Maybe you just have to think of the complex square root in terms of the complex exponential and logarithm, at which point it becomes OK for canonicalize_radical()
to affect certain log/exp expressions?
comment:23 follow-up: ↓ 24 Changed 8 years ago by
In def simplify
,
.. seealso:: :meth:`simplify_full`, :meth:`simplify_trig`, :meth:`simplify_rational`, :meth:`simplify_factorial`, :meth:`simplify_log`
You just removed it here rather than replacing it; intentional or not? It's still useful, could put a reference in.
(For that matter, we also now have several other simplify_foo
methods which should be added to the lists of simplifications in a few places.)
# Add elementwise methods. for method in ['simplify', 'simplify_exp', 'simplify_factorial', 'simplify_log', 'simplify_radical', 'simplify_rational', - 'simplify_trig', 'simplify_full', 'trig_expand', 'trig_reduce']: + 'simplify_trig', 'simplify_full', 'trig_expand', + 'canonicalize_radical', 'trig_reduce']: setattr(Vector_symbolic_dense, method, apply_map(getattr(Expression, method)))
Why is this keeping simplify_radical
?
Maybe you just have to think of the complex square root in terms of the complex exponential and logarithm, at which point it becomes OK for canonicalize_radical() to affect certain log/exp expressions?
Sure! But I mean that the name, if anything, is more about exp/log then as opposed to radicals. Just seems curious, and wondering whether a better name is possible. It may not be.
comment:24 in reply to: ↑ 23 Changed 8 years ago by
Replying to kcrisman:
In
def simplify
,.. seealso:: :meth:`simplify_full`, :meth:`simplify_trig`, :meth:`simplify_rational`, :meth:`simplify_factorial`, :meth:`simplify_log`You just removed it here rather than replacing it; intentional or not? It's still useful, could put a reference in.
Intentional, since it's no longer a simplify_*
method. But I'm fine with having it listed there.
(For that matter, we also now have several other
simplify_foo
methods which should be added to the lists of simplifications in a few places.)
I'm happy to add these. I've been meaning to add simplify_rectform()
to simplify_full()
once I stop making changes all over expression.pyx. Maybe in that ticket, or #14630 is probably where it's most appropriate since it adds yet another simplify_*
method.
# Add elementwise methods. for method in ['simplify', 'simplify_exp', 'simplify_factorial', 'simplify_log', 'simplify_radical', 'simplify_rational', - 'simplify_trig', 'simplify_full', 'trig_expand', 'trig_reduce']: + 'simplify_trig', 'simplify_full', 'trig_expand', + 'canonicalize_radical', 'trig_reduce']: setattr(Vector_symbolic_dense, method, apply_map(getattr(Expression, method)))Why is this keeping
simplify_radical
?
That code is adding methods to a class -- if we remove simplify_radical
entirely, it will break code that attempts to call simplify_radical
on a vector. Instead I leave it there to throw the deprecation warning.
comment:25 Changed 8 years ago by
- Commit changed from e5ab33994355520f0f27914f73f1680292eadc3d to 5d24f4c71b07beef87cc6b27df7f7cc37c29071e
Branch pushed to git repo; I updated commit sha1. New commits:
5d24f4c | Trac #11912 (review): Add canonicalize_radical() to the "see also" list for Expression.simplify().
|
comment:26 Changed 8 years ago by
Okay, I think it's probably time for this. Sadly. I'm happy with it, but building doc and testing yet to be done by me.
comment:27 Changed 8 years ago by
- Reviewers set to Karl-Dieter Crisman
Doc looks good! Now just waiting on make ptest
.
comment:28 Changed 8 years ago by
- Status changed from needs_review to positive_review
comment:29 Changed 8 years ago by
Thanks for taking the time to review this!
comment:30 Changed 8 years ago by
- Branch changed from u/mjo/ticket/11912 to 5d24f4c71b07beef87cc6b27df7f7cc37c29071e
- Resolution set to fixed
- Status changed from positive_review to closed
We should probably solve this at the same time as #12737.