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: |
Description
Get sage-4.0.rc0 from
http://sage.math.washington.edu/home/wstein/build/sage-4.0.rc0/dist/
which is stable and has all the new symbolics code in it.
You can also do:
./sage -upgrade http://sage.math.washington.edu/home/wstein/build/sage-4.0.rc0/
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)
Change History (15)
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
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
comment:3 Changed 9 years ago by
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/expression_conversions.py", 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/expression_conversions.py", 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
- Cc mhansen added
comment:5 Changed 9 years ago by
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: http://groups.google.com/group/sage-devel/msg/dc46d9d94dd31b5e (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
New package which adds error handling for py_get_constant() and changes the library version to 0.1.7 is here:
http://sage.math.washington.edu/home/burcin/pynac/pynac-0.1.7-r1.spkg
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 module_list.py. 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.
Cheers,
Burcin
Changed 9 years ago by
comment:7 Changed 9 years ago by
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/random_tests.py, 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 random_tests.py (which I can't review since I wrote it).
Changed 9 years ago by
comment:8 Changed 9 years ago by
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
comment:9 Changed 9 years ago by
- 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
- 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
- Resolution set to fixed
- Status changed from assigned to closed
Merged in 4.0.rc1.
TODO: This doctest in infinity.py was commented out. When uncommented we get a total hang:
This is what we get:
I'm uncomenting this in my referee patch.