Numerator for symbolic expression shouldn't use maxima

[hivert]

The code for numerator is currently

def numerator(self):
        """
        Returns the numerator of this symbolic expression.  If the
[...]
        """
        return self.parent()(self._maxima_().num())

Using Pynac should be much faster.

The patch wraps numer, denom, numer_denom and normal from GiNaC and fixes a
bunch of wrong sphinx markup in expression.pyx.

Apply attachment: trac_12068-numer_denom_normal-ginac-fh.patch and attachment: trac_12068-reviewer.patch.

Component: symbolics

Keywords: numerator, denominator

Author: Florent Hivert, Burcin Erocal

Reviewer: Burcin Erocal, Florent Hivert, Karl-Dieter Crisman

Merged: sage-5.0.beta2

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

[hivert]

comment:1

Ginac's behavior is not the same has Maxima: given 1 + 1/(x + 1)

• Maxima returns 1 + 1/(x + 1) as numerator and 1 as denominator;

• Ginac returns x + 2 as numerator and x + 1 as denominator.

I think both are useful. My patch keeps the current behavior. Is this what we want ?

Florent

[hivert]

Author: Florent Hivert

[burcin]

Reviewer: Burcin Erocal

[burcin]

comment:3

Looks good to me. It would be better to use elif in line 6480 and 6481. Otherwise, positive review once the tests pass.

Thank you for working on this.

[hivert]

Attachment: trac_12068-numer_denom_ginac-fh.patch.gz

[hivert]

comment:4

Replying to @burcin:

Looks good to me. It would be better to use elif in line 6480 and 6481. Otherwise, positive review once the tests pass.

Done !

[hivert]

comment:5

I got a all test passed on my laptop except a timeout in sage/schemes/elliptic_curves/ell_rational_field.py however relaunching a non parallel test on this single file gives:

sage -t  "sage/schemes/elliptic_curves/ell_rational_field.py"
         [89.8 s]
 
----------------------------------------------------------------------
All tests passed!
Total time for all tests: 89.8 seconds

[burcin]

comment:6

Attachment: trac_12068-denominator.patch.gz

Declaring py_pow as an int caused problems with attachment: trac_12068-numer_denom_ginac-fh.patchpy_object_from_numeric() returns a PyObject. Assigning the return value to an int worked because Cython was creating a temporary PyObject and trying to convert that to an int. This failed if the exponent was not an integer.

attachment: trac_12068-denominator.patch fixes this problem and handles expressions which contain only a power.

Florent can you review my patch?

Apply attachment: trac_12068-numer_denom_ginac-fh.patch, attachment: trac_12068-denominator.patch

[burcin]

Changed author from Florent Hivert to Florent Hivert, Burcin Erocal

[hivert]

comment:7

Replying to @burcin:

Florent can you review my patch?

Thanks for fixing my mistake. Unfortunately, because I choose to duplicate the code to speedup numerator_denominator, I also duplicate the mistake. You corrected only one. I'll upload a patch fixing everything.

[hivert]

Attachment: trac_12068-numer_denom_fix-fh.patch.gz

[hivert]

comment:8

Attachment: trac_12068-numer_denom_ginac-folded-fh.patch.gz

Hi Burcin,

The uploaded patch should fix everything. attachment: trac_12068-numer_denom_fix-fh.patch
contains my modifications on top of yours and attachment: trac_12068-numer_denom_ginac-folded-fh.patch contains everything folded.

Your turn to review ;-)

[burcin]

comment:9

It all looks good. The declaration cdef int py_pow is redundant in numerator_denominator(), but I'll switch to positive review anyway.

[burcin]

Changed reviewer from Burcin Erocal to Burcin Erocal, Florent Hivert

[hivert]

comment:10

Bi Burcin,

Replying to @burcin:

It all looks good. The declaration cdef int py_pow is redundant in numerator_denominator(), but I'll switch to positive review anyway.

The following diff

diff --git a/sage/symbolic/expression.pyx b/sage/symbolic/expression.pyx
--- a/sage/symbolic/expression.pyx
+++ b/sage/symbolic/expression.pyx
@@ -1867,7 +1867,7 @@ cdef class Expression(CommutativeRingEle
         assured that if True or False is returned (and proof is False) then 
         the answer is correct. 
 
-        INPUT::
+        INPUT:
         
            ntests -- (default 20) the number of iterations to run
            domain -- (optional) the domain from which to draw the random values

broke the doc. So I had to fix my patch. Doing so I discovered a few more typos and fixed them once for all. In the process I ended up folding the patch for #12072.

So please re review. Sorry for the double review.

Florent

[hivert]

Attachment: trac_12068-numer_denom_normal-ginac-fh.patch.gz

[hivert]

comment:11

Please re-review. Compared to my previous patch, I

• wrapped normal;
• removed the unused py_pow declaration;
• fixed a bunch of doc typos.

Again sorry for the extra work,

Florent

[kcrisman]

Changed reviewer from Burcin Erocal, Florent Hivert to Burcin Erocal, Florent Hivert, Karl-Dieter Crisman

[kcrisman]

comment:12

The changes to the previous patch seem fine, docs are good, tests pass. In fact, it's a very nice patch.

The only problem I spied is in the last hunk:

         - ``self`` -- the symbolic expression converting from 
         - ``target`` -- (default None) the symbolic expression

is too far indented. If you're going to fix all of this stuff, you might as well make these at the same indentation level as INPUT, such as in the second-to-last hunk.

In fact, I'm attaching a reviewer patch to fix this.

I wonder if there is a more 'obvious' name for normalize that could be an alias... anyway, not for this ticket. Assuming that Burcin has no objections, positive review other than this.

[kcrisman]

Attachment: trac_12068-reviewer.patch.gz

reviewer patch

[kcrisman]

comment:13

Apply attachment: trac_12068-numer_denom_normal-ginac-fh.patch and attachment: trac_12068-reviewer.patch.

[hivert]

comment:14

Hi Karl-Dieter,

Replying to @kcrisman:

The changes to the previous patch seem fine, docs are good, tests pass. In fact, it's a very nice patch.

Thanks !

The only problem I spied is in the last hunk:

         - ``self`` -- the symbolic expression converting from 
         - ``target`` -- (default None) the symbolic expression

is too far indented. If you're going to fix all of this stuff, you might as well make these at the same indentation level as INPUT, such as in the second-to-last hunk.

In fact, I'm attaching a reviewer patch to fix this.

Should't someone review your trivial reviewer patch before putting positive review ? Anyway, It is an obvious patch and I did review it. So I confirm your positive review.

I wonder if there is a more 'obvious' name for normalize that could be an alias... anyway, not for this ticket. Assuming that Burcin has no objections, positive review other than this.

In every CAS I used, I've always been confused by simplify, normal, combine... I guess Sage isn't an exception.

[kcrisman]

comment:15

In fact, I'm attaching a reviewer patch to fix this.

Should't someone review your trivial reviewer patch before putting positive review ? Anyway, It is an obvious patch and I did review it. So I confirm your positive review.

No, reviewers are usually allowed to make VERY trivial changes, esp. to fix doc, without 'formal' other review, otherwise we would take even longer to review things than normal. Naturally, anyone could decide that "reviewer patch X needs review" if they felt it was nontrivial.

[jdemeyer]

Merged: sage-5.0.beta2