Opened 11 years ago
Closed 11 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 11 years ago by
comment:2 Changed 11 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 11 years ago by
comment:3 Changed 11 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 11 years ago by
- Cc mhansen added
comment:5 Changed 11 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 11 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 11 years ago by
comment:7 Changed 11 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 11 years ago by
comment:8 Changed 11 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 11 years ago by
comment:9 Changed 11 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 11 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 11 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.