#12950 closed defect (fixed)
update to Pynac 0.2.4
Reported by: | burcin | Owned by: | burcin |
---|---|---|---|
Priority: | major | Milestone: | sage-5.1 |
Component: | symbolics | Keywords: | pynac sd40.5 |
Cc: | titusn, benjaminfjones, kcrisman, jpflori | Merged in: | sage-5.1.beta2 |
Authors: | Volker Braun, Burcin Erocal, Jean-Pierre Flori, Titus Nicolae, Alexei Sheplyakov | Reviewers: | Jean-Pierre Flori, Burcin Erocal, Benjamin Jones |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
There is a new Pynac release which contains fixes to:
- #11155:
abs(pi+I) -> pi + I
(upstream from Alexei Sheplyakov) - #11423:
atan2(0,0)
should be undefined (by Volker Braun) - #11919: pickling of user defined symbolic functions without custom methods
- #12303: beta should remain symbolic for exact input
- a fix to use Python for comparing Python objects wrapped in numerics (by Jean-Pierre Flori
- complete rewrite of code handling various types of infinity (by Volker Braun)
Following the comments this thread on sage-devel, the release without the upstream mercurial directories is available as a tarball:
http://sage.math.washington.edu/home/burcin/pynac/pynac-0.2.4.tar.bz2
A tentative spkg is available at:
http://perso.telecom-paristech.fr/~flori/sage/pynac-0.2.4.spkg
It also adds an .hgignore file to exclude tracking of the src directory.
Apply:
- attachment:trac_12950-pickle_symbolic_function-trac.patch
- attachment:trac_11423-atan_error-trac.patch
- attachment:trac_12950-revolution_plot3d.patch
- attachment:trac_12950-numeric_comparison_doctest_fixes-trac.patch
- attachment:trac_12950-further_doctests_for_numerics.patch
- attachment:trac_12950-symbolic_beta-trac.patch
- attachment:trac_12950-pynac_infinities-trac.patch
- attachment:trac_12950-pynac_infinities_doctest_fixes-trac.patch
- attachment:trac_12950-psi_evalf-trac.patch
- attachment:trac_12950-reviewer.patch
Attachments (17)
Change History (43)
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
Changed 8 years ago by
comment:1 Changed 8 years ago by
- Description modified (diff)
comment:2 Changed 8 years ago by
comment:3 Changed 8 years ago by
- Cc benjaminfjones added
comment:4 Changed 8 years ago by
- Cc kcrisman added
comment:5 Changed 8 years ago by
- Cc jpflori added
comment:6 Changed 8 years ago by
- Description modified (diff)
- Reviewers set to Jean-Pierre Flori
- Work issues changed from make spkg to review spkg, correct some typos in the patches
I've made a tentative spkg available at http://perso.telecom-paristech.fr/~flori/sage/pynac-0.2.4.spkg which adds an .hgignore file to exclude the src directory from tracking.
I've had a quick look at the patches which look good overall. I'll post a patch fixing some (mainly very minor) typos asap.
comment:7 Changed 8 years ago by
I'm currently running "make ptest" (on sage-5.0 on an ubuntu 12.04 amd64) and there are at least some new problems in revolutio_plot3d (or something like that) because atan2(0,0) now raises an error.
comment:8 Changed 8 years ago by
I also got two failures in sage/stats/basic_stats due to the new ordering of numerics (I guess).
comment:9 follow-up: ↓ 10 Changed 8 years ago by
Oh, so now I'm a little lost. Some patches are included here and other on the original tickets... It's surely logical, but I just discovered that.
Anyway, the dependencies between the tickets should be clearly stated and written into the corresponding trac field. For example, do we really want to merge #11919 now? cos I don't think reviewing this ticket will take ages... Or, this ticket can not be merged without fixing the atan2 stuff causing failures in revolution_plot3d which is in fact also mentioned in the comments of #11423 . Or, do we really want the three lines patch of #11155 there with #12950 as a dependency rather than here?
comment:10 in reply to: ↑ 9 Changed 8 years ago by
- Status changed from new to needs_review
Thanks for looking into this!
Replying to jpflori:
Oh, so now I'm a little lost. Some patches are included here and other on the original tickets... It's surely logical, but I just discovered that.
I tried to put all the patches required to make the new version work with Sage on this ticket. If I didn't screw up, the rest should be nonessential, doctests, etc. It used to be a major hassle to create patches for each ticket separately, upload them, then make a ticket with an spkg etc. I think this way is easier for everyone.
Anyway, the dependencies between the tickets should be clearly stated and written into the corresponding trac field. For example, do we really want to merge #11919 now? cos I don't think reviewing this ticket will take ages...
I don't think the patch attached to #11919 is necessary. But I'm reluctant to make changes to that ticket while this is being reviewed.
Or, this ticket can not be merged without fixing the atan2 stuff causing failures in revolution_plot3d which is in fact also mentioned in the comments of #11423 .
This should depend on the yet to be opened ticket mentioned in comment:6:ticket:11423. But, changing the plot3d stuff to handle errors in the numerical evaluation phase is a big change (AFAIK, Titus actually had a go at it.), and it is very much independent of a Pynac release, apart from the minor issue that one of the doctests there happened to call atan2()
.
Or, do we really want the three lines patch of #11155 there with #12950 as a dependency rather than here?
The doctests on #11155 are minor, so we left that patch out of this ticket. Just now, I made that ticket depend on this one and gave it a positive review.
comment:11 Changed 8 years ago by
I've just uploaded a fix for the revolution_plot3d problem.
When the revolution axis goes through (0,0,0), the previous behaviour was correct although obtained through the general formula which in fact should not have made sense. In fact the (0,0,0) case is the simplest one, no "complicated" formula for the to be drawn surface has to be deduced from the curve formula. The proposed fix is to treat the special case (0,0,0) separately as far as the phase calculation is concerned.
(PS: We posted more or less at the same time, so this post might seem strange)
comment:12 Changed 7 years ago by
A little concern about the effect of the new numeric ordering, if I understand correctly what happens (not really sure about the initialization of the complex I). Now the complex I is compared at the Python level. There it is a root of x2+1, and Sage always returns 1 when you compare whatever*I and whatever_different*I, that is : 5*I > 3*I is true, but so is I > 3*I.
This explains the changes in some doctest output, but I'm concerned that this will surely lead to problems in the pynac internal ordering...
comment:13 Changed 7 years ago by
Further rants: in fact in (quadratic) number fields at least we always have whatever > whatever else true and whatever < whatever else false. Indeed, the _cmp_ functions test for equality, that is completely understandable because there's no reason to decide that a root of a polynomial is "positive" or "negative" without chosing an embedding somewhere...
But I is defined with embedding=CC.gen(), i.e. the usual definition, and so we have CC(I) considered as "positive" (no surprise here...).
comment:14 follow-up: ↓ 17 Changed 7 years ago by
I think that the above could or even should be handled in a separate ticket (#9880 for example...).
In a few minutes, I'll upload a reviewer patch fixing one or two very minor typos and another patch fixing the two other doctests modified by the complex I ordering changes.
I'll also upload new versions of all the other patches so that the commit messages include the trac ticket number.
Changed 7 years ago by
Changed 7 years ago by
Changed 7 years ago by
Changed 7 years ago by
Changed 7 years ago by
Changed 7 years ago by
comment:15 Changed 7 years ago by
- Description modified (diff)
Changed 7 years ago by
comment:16 Changed 7 years ago by
I think my work here is done. I positively review Burcin and others' patches.
So if one is ok with the packaging I made and the few other patches I uploaded, this can get positive review.
comment:17 in reply to: ↑ 14 Changed 7 years ago by
- Reviewers changed from Jean-Pierre Flori to Jean-Pierre Flori, Burcin Erocal
- Work issues changed from review spkg, correct some typos in the patches to review spkg
Replying to jpflori:
I think that the above could or even should be handled in a separate ticket (#9880 for example...).
This is actually #7160. It causes a lot of other problems in Pynac as well.
I give a positive review to Jean-Pierre's patches. The spkg still needs to be reviewed. Any volunteers?
comment:18 Changed 7 years ago by
- Reviewers changed from Jean-Pierre Flori, Burcin Erocal to Jean-Pierre Flori, Burcin Erocal, Benjamin Jones
Started reviewing the spkg. The package itself looks good, installs successfully, and with the attached patches applied, sage passes a subset of tests:
passes: ../../sage -t sage/symbolic/*.py passes: ../../sage -t sage/symbolic/*.pyx passes: ../../sage -t sage/functions/*.py
I'm running all tests now, will report when complete.
comment:19 Changed 7 years ago by
- Keywords sd40.5 added
- Status changed from needs_review to positive_review
All tests pass, I checked the hg log and indeed, all relevant changes appear to come from upstream. The spkg unpacks fine, and I can successfully recreate it using the sage-pkg
script.
Everything looks good, so I'll give the spkg a positive review.
comment:20 follow-up: ↓ 24 Changed 7 years ago by
While updating #11143 I ran into the following issue which occurs when the patches here and the new pynac spkg are installed:
sage: integral(e^(-x)*log(x+1), x, 0, oo) ... ValueError: Computation failed since Maxima requested additional constraints; using the 'assume' command before integral evaluation *may* help (example of legal syntax is 'as sume(Infinity>0)', see `assume?` for more details) Is Infinity positive, negative, or zero?
It looks like there is something going on with the conversion of oo
to inf
in maxima.
Another comment: why are the instances of Infinity
in the doctest patches here changed to infinity
. Isn't the upper case Infinity
the "canonical" Sage infinity?
comment:21 Changed 7 years ago by
- Work issues review spkg deleted
comment:22 Changed 7 years ago by
Could this have caused
sage -t -force_lib devel/sage/sage/rings/polynomial/polynomial_rational_flint.pyx ********************************************************************** File "/padic/scratch/jdemeyer/merger/sage-5.1.beta2/devel/sage-main/sage/rings/polynomial/polynomial_rational_flint.pyx", line 596: sage: f.reverse(I) Expected: Traceback (most recent call last): ... TypeError: can't convert complex to int; use int(abs(z)) Got: Traceback (most recent call last): File "/padic/scratch/jdemeyer/merger/sage-5.1.beta2/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/padic/scratch/jdemeyer/merger/sage-5.1.beta2/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/padic/scratch/jdemeyer/merger/sage-5.1.beta2/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_17[19]>", line 1, in <module> f.reverse(I)###line 596: sage: f.reverse(I) File "polynomial_rational_flint.pyx", line 608, in sage.rings.polynomial.polynomial_rational_flint.Polynomial_rational_flint.reverse (sage/rings/polynomial/polynomial_rational_flint.cpp:7922) len = <unsigned long> n File "expression.pyx", line 775, in sage.symbolic.expression.Expression.__int__ (sage/symbolic/expression.cpp:5027) ValueError: cannot convert I to int **********************************************************************
comment:23 Changed 7 years ago by
- Merged in set to sage-5.1.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
comment:24 in reply to: ↑ 20 ; follow-up: ↓ 25 Changed 7 years ago by
Replying to benjaminfjones:
While updating #11143 I ran into the following issue which occurs when the patches here and the new pynac spkg are installed:
sage: integral(e^(-x)*log(x+1), x, 0, oo) ... ValueError: Computation failed since Maxima requested additional constraints; using the 'assume' command before integral evaluation *may* help (example of legal syntax is 'as sume(Infinity>0)', see `assume?` for more details) Is Infinity positive, negative, or zero?It looks like there is something going on with the conversion of
oo
toinf
in maxima.
Another comment: why are the instances of
Infinity
in the doctest patches here changed toinfinity
. Isn't the upper caseInfinity
the "canonical" Sage infinity?
Did you want to open a ticket about these, or was this just a point of clarification?
On an unrelated note, I'm a little confused as to why all the patches ended up here. I understand that this is convenient, but what happens then is that a whole bunch of tickets are closed as "sage-invalid/dup" and really they weren't dups at all, they just happened to be fixed by this spkg.
At any rate, I recommend making sure that such tickets (like #11423, #9547, #12303) are given a normal milestone - even though the fix is here, that didn't make the ticket and its authors/reviewers less valid somehow.
comment:25 in reply to: ↑ 24 ; follow-up: ↓ 26 Changed 7 years ago by
Replying to kcrisman:
On an unrelated note, I'm a little confused as to why all the patches ended up here. I understand that this is convenient, but what happens then is that a whole bunch of tickets are closed as "sage-invalid/dup" and really they weren't dups at all, they just happened to be fixed by this spkg.
At any rate, I recommend making sure that such tickets (like #11423, #9547, #12303) are given a normal milestone - even though the fix is here, that didn't make the ticket and its authors/reviewers less valid somehow.
Feel free to change them back to a different milestone. I also hesitated before changing to duplicate/invalid/wontfix. I just needed a way to indicate to Jeroen that these are tickets that can be closed without any work. I also tried to make the authors of those tickets were acknowledged here.
comment:26 in reply to: ↑ 25 Changed 7 years ago by
On an unrelated note, I'm a little confused as to why all the patches ended up here. I understand that this is convenient, but what happens then is that a whole bunch of tickets are closed as "sage-invalid/dup" and really they weren't dups at all, they just happened to be fixed by this spkg.
At any rate, I recommend making sure that such tickets (like #11423, #9547, #12303) are given a normal milestone - even though the fix is here, that didn't make the ticket and its authors/reviewers less valid somehow.
Feel free to change them back to a different milestone. I also hesitated before changing to duplicate/invalid/wontfix. I just needed a way to indicate to Jeroen that these are tickets that can be closed without any work. I also tried to make the authors of those tickets were acknowledged here.
I sort of figured that was the situation - there is no perfect workflow for all needs, by Arrow's Theorem, and it's not like you don't have enough to do! I'll check if any changes would be required.
This also fixes #9547 and includes a doctest. That ticket should be closed once this is merged.