Opened 9 years ago
Closed 9 years ago
#12068 closed enhancement (fixed)
Numerator for symbolic expression shouldn't use maxima
Reported by:  hivert  Owned by:  hivert 

Priority:  major  Milestone:  sage5.0 
Component:  symbolics  Keywords:  numerator, denominator 
Cc:  Merged in:  sage5.0.beta2  
Authors:  Florent Hivert, Burcin Erocal  Reviewers:  Burcin Erocal, Florent Hivert, KarlDieter Crisman 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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 trac_12068numer_denom_normalginacfh.patch and trac_12068reviewer.patch.
Attachments (6)
Change History (22)
comment:1 Changed 9 years ago by
 Owner changed from burcin to hivert
comment:2 Changed 9 years ago by
 Status changed from new to needs_review
comment:3 followup: ↓ 4 Changed 9 years ago by
 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 9 years ago by
comment:4 in reply to: ↑ 3 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
comment:6 followup: ↓ 7 Changed 9 years ago by
 Description modified (diff)
Declaring py_pow
as an int
caused problems with attachment:trac_12068numer_denom_ginacfh.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_12068denominator.patch fixes this problem and handles expressions which contain only a power
.
Florent can you review my patch?
Apply attachment:trac_12068numer_denom_ginacfh.patch, attachment:trac_12068denominator.patch
comment:7 in reply to: ↑ 6 Changed 9 years ago by
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 9 years ago by
Changed 9 years ago by
comment:8 Changed 9 years ago by
 Description modified (diff)
Hi Burcin,
The uploaded patch should fix everything. attachment:trac_12068numer_denom_fixfh.patch contains my modifications on top of yours and attachment:trac_12068numer_denom_ginacfoldedfh.patch contains everything folded.
Your turn to review ;)
comment:9 followup: ↓ 10 Changed 9 years ago by
 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 9 years ago by
 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 innumerator_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 1867 1867 assured that if True or False is returned (and proof is False) then 1868 1868 the answer is correct. 1869 1869 1870 INPUT: :1870 INPUT: 1871 1871 1872 1872 ntests  (default 20) the number of iterations to run 1873 1873 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
Changed 9 years ago by
comment:11 Changed 9 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
Please rereview. 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
comment:12 followup: ↓ 14 Changed 9 years ago by
 Reviewers changed from Burcin Erocal, Florent Hivert to Burcin Erocal, Florent Hivert, KarlDieter Crisman
 Status changed from needs_review to positive_review
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 secondtolast 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.
comment:13 Changed 9 years ago by
 Description modified (diff)
comment:14 in reply to: ↑ 12 ; followup: ↓ 15 Changed 9 years ago by
Hi KarlDieter,
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 secondtolast 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.
comment:15 in reply to: ↑ 14 Changed 9 years ago by
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.
comment:16 Changed 9 years ago by
 Merged in set to sage5.0.beta2
 Resolution set to fixed
 Status changed from positive_review to closed
Ginac's behavior is not the same has Maxima: given
1 + 1/(x + 1)
1 + 1/(x + 1)
as numerator and1
as denominator;x + 2
as numerator andx + 1
as denominator.I think both are useful. My patch keeps the current behavior. Is this what we want ?
Florent