Opened 10 years ago

Last modified 10 years ago

#12068 closed enhancement

Numerator for symbolic expression shouldn't use maxima — at Version 11

Reported by: hivert Owned by: hivert
Priority: major Milestone: sage-5.0
Component: symbolics Keywords: numerator, denominator
Cc: Merged in:
Authors: Florent Hivert, Burcin Erocal Reviewers: Burcin Erocal, Florent Hivert
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by 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

Change History (16)

comment:1 Changed 10 years ago by hivert

  • Authors set to Florent Hivert
  • Owner changed from burcin to hivert

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

comment:2 Changed 10 years ago by hivert

  • Status changed from new to needs_review

comment:3 follow-up: Changed 10 years ago by burcin

  • Reviewers set to Burcin Erocal
  • Summary changed from Numerator for symbolic expression should'nt use maxima to Numerator for symbolic expression shouldn't use maxima

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.

Changed 10 years ago by hivert

comment:4 in reply to: ↑ 3 Changed 10 years ago by hivert

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 !

comment:5 Changed 10 years ago by hivert

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

Changed 10 years ago by burcin

comment:6 follow-up: Changed 10 years ago by burcin

  • Authors changed from Florent Hivert to Florent Hivert, Burcin Erocal
  • Description modified (diff)

Declaring py_pow as an int caused problems with attachment:trac_12068-numer_denom_ginac-fh.patch. py_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

comment:7 in reply to: ↑ 6 Changed 10 years ago by hivert

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.

Changed 10 years ago by hivert

comment:8 Changed 10 years ago by hivert

  • Description modified (diff)

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

comment:9 follow-up: Changed 10 years ago by burcin

  • Reviewers changed from Burcin Erocal to Burcin Erocal, Florent Hivert
  • Status changed from needs_review to positive_review

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

comment:10 in reply to: ↑ 9 Changed 10 years ago by hivert

  • Status changed from positive_review to needs_work

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

  • sage/symbolic/expression.pyx

    diff --git a/sage/symbolic/expression.pyx b/sage/symbolic/expression.pyx
    a b cdef class Expression(CommutativeRingEle 
    18671867        assured that if True or False is returned (and proof is False) then
    18681868        the answer is correct.
    18691869
    1870         INPUT::
     1870        INPUT:
    18711871       
    18721872           ntests -- (default 20) the number of iterations to run
    18731873           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

comment:11 Changed 10 years ago by hivert

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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

Note: See TracTickets for help on using tickets.