Opened 8 years ago

Closed 5 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) Commit: 5d24f4c71b07beef87cc6b27df7f7cc37c29071e
Dependencies: Stopgaps:

Description (last modified by mjo)

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)

sage-trac_11912.patch (8.0 KB) - added by mjo 6 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 6 years ago by kcrisman

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

comment:2 Changed 6 years ago by mjo

  • Description modified (diff)

comment:3 in reply to: ↑ description Changed 6 years ago by 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.

comment:4 Changed 6 years ago by mjo

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 6 years ago by mjo

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

Changed 6 years ago by mjo

comment:6 Changed 6 years ago by mjo

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 6 years ago by mjo

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 6 years ago by kcrisman

  • 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 6 years ago by kcrisman

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

Last edited 5 years ago by kcrisman (previous) (diff)

comment:10 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:11 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:12 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:13 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 Changed 5 years ago by mjo

  • Authors set to Michael Orlitzky
  • 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:

f34ddfcTrac #11912: Rename and deprecate Expression.simplify_radical().

comment:15 Changed 5 years ago by kcrisman

Probably see also #3520.

comment:16 Changed 5 years ago by kcrisman

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 5 years ago by git

  • Commit changed from f34ddfc2e47508ebc2323e347739f18000c15717 to 2d50b886561b9cc98585749b6292eaba269da73c

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

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

comment:18 follow-up: Changed 5 years ago by mjo

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.

Last edited 5 years ago by mjo (previous) (diff)

comment:19 in reply to: ↑ 18 ; follow-up: Changed 5 years ago by kcrisman

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

comment:20 Changed 5 years ago by kcrisman

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 5 years ago by git

  • Commit changed from 2d50b886561b9cc98585749b6292eaba269da73c to e5ab33994355520f0f27914f73f1680292eadc3d

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

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

comment:22 in reply to: ↑ 19 Changed 5 years ago by mjo

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: Changed 5 years ago by 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.

(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 5 years ago by mjo

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 5 years ago by git

  • Commit changed from e5ab33994355520f0f27914f73f1680292eadc3d to 5d24f4c71b07beef87cc6b27df7f7cc37c29071e

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

5d24f4cTrac #11912 (review): Add canonicalize_radical() to the "see also" list for Expression.simplify().

comment:26 Changed 5 years ago by kcrisman

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 5 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman

Doc looks good! Now just waiting on make ptest.

comment:28 Changed 5 years ago by kcrisman

  • Status changed from needs_review to positive_review

comment:29 Changed 5 years ago by mjo

Thanks for taking the time to review this!

comment:30 Changed 5 years ago by vbraun

  • Branch changed from u/mjo/ticket/11912 to 5d24f4c71b07beef87cc6b27df7f7cc37c29071e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.