Clarify simplify_radical and Maxima's radcan

[kcrisman]

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.

CC: @burcin @zimmermann6

Component: documentation

Keywords: sd48

Author: Michael Orlitzky

Branch/Commit: 5d24f4c

Reviewer: Karl-Dieter Crisman

Issue created by migration from https://trac.sagemath.org/ticket/11912

[kcrisman]

comment:1

We should probably solve this at the same time as #12737.

[burcin]

Replying to @kcrisman:

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.

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.

[orlitzky]

comment:4

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().

[orlitzky]

comment:5

Actually, I take that back -- canonicalize_radical() would be better for tab completion if we add other canonicalize_foo() methods.

[orlitzky]

comment:6

Attachment: sage-trac_11912.patch.gz

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.

[orlitzky]

comment:7

Note: I haven't updated the function name anywhere, so a ton of tests fail. Just looking for some ideas for now.

[kcrisman]

comment:8

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.

[kcrisman]

Changed keywords from none to sd48

[kcrisman]

comment:9

See this comment for both some good use cases and some potential new aliases...

[orlitzky]

Branch: u/mjo/ticket/11912

[orlitzky]

comment:14

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().

[orlitzky]

Commit: f34ddfc

[orlitzky]

Author: Michael Orlitzky

[kcrisman]

comment:15

Probably see also #3520.

[kcrisman]

comment:16

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.

[sagetrac-git]

Changed commit from f34ddfc to 2d50b88

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

2d50b88

Trac #11912, Trac #3520: Add numerical integral example.

[orlitzky]

comment:18

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.

[kcrisman]

comment:19

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.

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.)

[kcrisman]

comment:20

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.
+

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

e5ab339

Trac #11912 (review): Remove superfluous documentation paragraph.

[sagetrac-git]

Changed commit from 2d50b88 to e5ab339

[orlitzky]

comment:22

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?

[kcrisman]

comment:23

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.

[orlitzky]

comment:24

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.

[sagetrac-git]

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().

[sagetrac-git]

Changed commit from e5ab339 to 5d24f4c

[kcrisman]

comment:26

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.

[kcrisman]

comment:27

Doc looks good! Now just waiting on make ptest.

[kcrisman]

Reviewer: Karl-Dieter Crisman

[orlitzky]

comment:29

Thanks for taking the time to review this!

[vbraun]

Changed branch from u/mjo/ticket/11912 to 5d24f4c