update to Pynac 0.2.4

[burcin]

There is a new Pynac release which contains fixes to:

• abs(pi+I) = pi+I #11155: abs(pi+I) -> pi + I (upstream from Alexei Sheplyakov)
• Make atan2(0,0) consistent #11423: atan2(0,0) should be undefined (by Volker Braun)
• Issue when pickling a formal function #11919: pickling of user defined symbolic functions without custom methods
• leave beta symbolic for exact complex inputs #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 in 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

CC: @sagetrac-titusn @benjaminfjones @kcrisman @jpflori

Component: symbolics

Keywords: pynac sd40.5

Author: Volker Braun, Burcin Erocal, Jean-Pierre Flori, Titus Nicolae, Alexei Sheplyakov

Reviewer: Jean-Pierre Flori, Burcin Erocal, Benjamin Jones

Merged: sage-5.1.beta2

Issue created by migration from https://trac.sagemath.org/ticket/12950

[burcin]

Attachment: trac_12950-pickle_symbolic_function.patch.gz

[burcin]

Attachment: trac_11423-atan_error.patch.gz

[burcin]

Attachment: trac_12950-numeric_comparison_doctest_fixes.patch.gz

[burcin]

Attachment: trac_12950-symbolic_beta.patch.gz

[burcin]

Attachment: trac_12950-pynac_infinities.patch.gz

[burcin]

Attachment: trac_12950-pynac_infinities_doctest_fixes.patch.gz

[burcin]

comment:1

Attachment: trac_12950-psi_evalf.patch.gz

[burcin]

comment:2

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

[jpflori]

comment:6

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.

[jpflori]

Reviewer: Jean-Pierre Flori

[jpflori]

Changed work issues from make spkg to review spkg, correct some typos in the patches

[jpflori]

comment:7

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.

[jpflori]

comment:8

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

[jpflori]

comment:9

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?

[jpflori]

Fix for revolution_plot3d

[burcin]

comment:10

Attachment: trac_12950-revolution_plot3d.patch.gz

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 #11423 comment:6. 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.

[jpflori]

comment:11

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)

[jpflori]

comment:12

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 whateverI and whatever_differentI, that is : 5I > 3I 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...

[jpflori]

comment:13

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

[jpflori]

comment:14

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.

[jpflori]

Attachment: trac_11423-atan_error-trac.patch.gz

Further doctests fixes.

[jpflori]

Attachment: trac_12950-further_doctests_for_numerics.patch.gz

Attachment: trac_12950-pickle_symbolic_function-trac.patch.gz

[jpflori]

Attachment: trac_12950-psi_evalf-trac.patch.gz

[jpflori]

Attachment: trac_12950-pynac_infinities_doctest_fixes-trac.patch.gz

[jpflori]

Attachment: trac_12950-pynac_infinities-trac.patch.gz

[jpflori]

Attachment: trac_12950-symbolic_beta-trac.patch.gz

Reviewer patch; minor typos

[jpflori]

comment:15

Attachment: trac_12950-reviewer.patch.gz

[jpflori]

Attachment: trac_12950-numeric_comparison_doctest_fixes-trac.patch.gz

[jpflori]

comment:16

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.

[burcin]

comment:17

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?

[burcin]

Changed work issues from review spkg, correct some typos in the patches to review spkg

[burcin]

Changed reviewer from Jean-Pierre Flori to Jean-Pierre Flori, Burcin Erocal

[benjaminfjones]

Changed reviewer from Jean-Pierre Flori, Burcin Erocal to Jean-Pierre Flori, Burcin Erocal, Benjamin Jones

[benjaminfjones]

comment:18

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.

[benjaminfjones]

Changed keywords from pynac to pynac sd40.5

[benjaminfjones]

comment:19

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.

[benjaminfjones]

comment:20

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?

[jdemeyer]

Changed work issues from review spkg to none

[jdemeyer]

Merged: sage-5.1.beta2

[kcrisman]

comment:24

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.

[burcin]

comment:25

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.

[kcrisman]

comment:26

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.