Opened 9 years ago

Closed 9 years ago

#6111 closed defect (fixed)

[with patch; positive review] review symbolics in sage-4.0 (switch to pynac)

Reported by: was Owned by: mhansen
Priority: blocker Milestone: sage-4.0
Component: calculus Keywords:
Cc: mhansen Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:


Get sage-4.0.rc0 from

which is stable and has all the new symbolics code in it.

You can also do:

./sage -upgrade

Note that the code for symbolics is one big flattened patch. The only way to referee it is is to read straight through all of devel/sage/sage/symbolics, devel/sage/sage/calculus, and also look at the diff outside of those two directories at the patch to see what else was changed.

Attachments (4)

6111-expression-referee.patch (7.2 KB) - added by robertwb 9 years ago.
trac6111-reviewer-cwitty.patch (17.2 KB) - added by cwitty 9 years ago.
trac_6111-stein_referee.patch (14.8 KB) - added by was 9 years ago.
trac_6111-sympy_oo.patch (1.9 KB) - added by mhansen 9 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by was

TODO: This doctest in was commented out. When uncommented we get a total hang:

            sage: bool(SR(oo) == sympy.oo)

This is what we get:

sage:             sage: bool(SR(oo) == sympy.oo)
terminate called after throwing an instance of 'std::runtime_error'
  what():  indeterminate expression: Infinity - Infinity encountered.


I'm uncomenting this in my referee patch.

comment:2 Changed 9 years ago by robertwb

I read through the first 2500 lines of expression.pyx earlier today, and it's looking good so far (collecting minor fixes into a patch). I won't be looking at it again until much later tonight.

Changed 9 years ago by robertwb

comment:3 Changed 9 years ago by robertwb

Finished reading expression.pyx, it looks good. Should this work:

sage: ((x+y)^9).polynomial(None, ring=ZZ['x']['y']).list()
Traceback (most recent call last):
  File "<ipython console>", line 1, in <module>
  File "expression.pyx", line 3616, in sage.symbolic.expression.Expression.polynomial (sage/symbolic/expression.cpp:17320)
  File "/home/robertwb/sage-4.0.rc0-x86_64-Linux/local/lib/python2.5/site-packages/sage/symbolic/", line 975, in polynomial
    converter = PolynomialConverter(ex, base_ring=base_ring, ring=ring)
  File "/home/robertwb/sage-4.0.rc0-x86_64-Linux/local/lib/python2.5/site-packages/sage/symbolic/", line 833, in __init__
    raise TypeError, "%s is not a variable of %s" %(v, ring)
TypeError: y is not a variable of Univariate Polynomial Ring in y over Univariate Polynomial Ring in x over Integer Ring

Also, there are a lot (too many IMHO) aliases for simplify_radical

    radical_simplify = simplify_log = log_simplify = simplify_radical
    simplify_exp = exp_simplify = simplify_radical

comment:4 Changed 9 years ago by robertwb

  • Cc mhansen added

comment:5 Changed 9 years ago by burcin

Some comments after reading the changesets to pynac after the unreleased 0.1.6 version (numbers next to the bullets refer to changesets in the repository from the pynac-0.1.7.spkg file):

  • 90:fb6582f9bd81

isn't this already handled by the first lines of pyExpression_to_ex() in pynac.pyx?

  • 88:25d02eeb427d

we should add error checking to py_get_constant() call, we can't guarantee that a constant with the same name will be available between different versions of Sage

  • 84:7b3272f4b4bf is mine, author field lists Mike
  • 82:da7d7a220723 evalf for constants using a lookup table

I already commented on this on IRC and the devel list. ginac/pynac already provides a framework to handle this, I would like to revert this in a future version. My previous message: (scroll to the paragraph below the bullet items)

I will try to prepare a new package which fixes the version numbers of the library and error handling of py_get_constant().

comment:6 Changed 9 years ago by burcin

New package which adds error handling for py_get_constant() and changes the library version to 0.1.7 is here:

Changing the version numbers of the library might expose some modules in the Sage library which should be rebuilt when the package is updated. The fix is to add a dependency to the pynac header in I didn't try to see which files need this, since my tree wasn't clean and all the relevant files (sage/symbolic/* is more than enough) got rebuilt for me.

Answering my own comments above, I see that the patch I sent Mike for changeset 84 didn't have hg headers, and returning None from eval or derivative methods is a good trick to halt the evaluation. The evalf function for constants is still as it is, I don't think it's a good idea to introduce big changes at this stage.

Thanks everyone for all the work to make the switch, I still can't believe we're almost there.



Changed 9 years ago by cwitty

comment:7 Changed 9 years ago by cwitty

I like it! I quickly read through all of sage/symbolic except for expression.pyx, and attached a patch fixing the issues I found. Technically somebody else should review sage/symbolic/, since I wrote the original version; but if you do, be sure to review rc0+my reviewer patch, since the reviewer patch adds doctests.

So: positive review for sage/symbolic/* except for expression.pyx (which I didn't look at) and (which I can't review since I wrote it).

Changed 9 years ago by was

comment:8 Changed 9 years ago by was

I read through most everything outside of symbolics/ and made a few little touchups in my attached patch. Positive review for this part as well.

Changed 9 years ago by mhansen

comment:9 Changed 9 years ago by mhansen

  • Owner changed from burcin to mhansen
  • Status changed from new to assigned

I looked over the first three patches, and they look good to me.

I added a fourth patch which fixes a segfault that wstein's patch uncovers.

comment:10 Changed 9 years ago by was

  • Summary changed from [with patch; needs review] review symbolics in sage-4.0 (switch to pynac) to [with patch; positive review] review symbolics in sage-4.0 (switch to pynac)

Regarding, sage: ((x+y)^9).polynomial(None, ring=ZZ['x']['y']).list() --> boom, none of the above patches address this. However, this ring option didn't even exist in sage < 4.0, so I'm not going to hold this ticket up for this.

comment:11 Changed 9 years ago by mhansen

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged in 4.0.rc1.

Note: See TracTickets for help on using tickets.