Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#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 jpflori)

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:

Attachments (17)

trac_12950-pickle_symbolic_function.patch (2.9 KB) - added by burcin 8 years ago.
trac_11423-atan_error.patch (2.7 KB) - added by burcin 8 years ago.
trac_12950-numeric_comparison_doctest_fixes.patch (2.4 KB) - added by burcin 8 years ago.
trac_12950-symbolic_beta.patch (1.0 KB) - added by burcin 8 years ago.
trac_12950-pynac_infinities.patch (7.0 KB) - added by burcin 8 years ago.
trac_12950-pynac_infinities_doctest_fixes.patch (8.7 KB) - added by burcin 8 years ago.
trac_12950-psi_evalf.patch (794 bytes) - added by burcin 8 years ago.
trac_12950-revolution_plot3d.patch (1.5 KB) - added by jpflori 8 years ago.
Fix for revolution_plot3d
trac_11423-atan_error-trac.patch (2.7 KB) - added by jpflori 7 years ago.
trac_12950-further_doctests_for_numerics.patch (1.2 KB) - added by jpflori 7 years ago.
Further doctests fixes.
trac_12950-pickle_symbolic_function-trac.patch (2.9 KB) - added by jpflori 7 years ago.
trac_12950-psi_evalf-trac.patch (820 bytes) - added by jpflori 7 years ago.
trac_12950-pynac_infinities_doctest_fixes-trac.patch (8.7 KB) - added by jpflori 7 years ago.
trac_12950-pynac_infinities-trac.patch (7.0 KB) - added by jpflori 7 years ago.
trac_12950-symbolic_beta-trac.patch (1.0 KB) - added by jpflori 7 years ago.
trac_12950-reviewer.patch (1.7 KB) - added by jpflori 7 years ago.
Reviewer patch; minor typos
trac_12950-numeric_comparison_doctest_fixes-trac.patch (2.4 KB) - added by jpflori 7 years ago.

Download all attachments as: .zip

Change History (43)

Changed 8 years ago by burcin

Changed 8 years ago by burcin

Changed 8 years ago by burcin

Changed 8 years ago by burcin

Changed 8 years ago by burcin

comment:1 Changed 8 years ago by burcin

  • Description modified (diff)

comment:2 Changed 8 years ago by burcin

This also fixes #9547 and includes a doctest. That ticket should be closed once this is merged.

comment:3 Changed 8 years ago by benjaminfjones

  • Cc benjaminfjones added

comment:4 Changed 8 years ago by kcrisman

  • Cc kcrisman added

comment:5 Changed 8 years ago by jpflori

  • Cc jpflori added

comment:6 Changed 8 years ago by jpflori

  • 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 jpflori

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 jpflori

I also got two failures in sage/stats/basic_stats due to the new ordering of numerics (I guess).

comment:9 follow-up: Changed 8 years ago by 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.

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?

Changed 8 years ago by jpflori

Fix for revolution_plot3d

comment:10 in reply to: ↑ 9 Changed 8 years ago by burcin

  • 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 jpflori

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 jpflori

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 jpflori

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: Changed 7 years ago by jpflori

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 jpflori

Changed 7 years ago by jpflori

Further doctests fixes.

Changed 7 years ago by jpflori

Changed 7 years ago by jpflori

Changed 7 years ago by jpflori

Changed 7 years ago by jpflori

Reviewer patch; minor typos

comment:15 Changed 7 years ago by jpflori

  • Description modified (diff)

comment:16 Changed 7 years ago by jpflori

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 burcin

  • 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 benjaminfjones

  • 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 benjaminfjones

  • 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: Changed 7 years ago by 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 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 jdemeyer

  • Work issues review spkg deleted

comment:22 Changed 7 years ago by jdemeyer

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
**********************************************************************
Version 0, edited 7 years ago by jdemeyer (next)

comment:23 Changed 7 years ago by jdemeyer

  • 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: Changed 7 years ago by kcrisman

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 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?

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: Changed 7 years ago by burcin

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 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.

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.

Note: See TracTickets for help on using tickets.