Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to pynac-0.6.91 #21623

Closed
rwst opened this issue Oct 2, 2016 · 27 comments
Closed

Upgrade to pynac-0.6.91 #21623

rwst opened this issue Oct 2, 2016 · 27 comments

Comments

@rwst
Copy link

rwst commented Oct 2, 2016

https://github.com/pynac/pynac/releases/download/pynac-0.6.91/pynac-0.6.91.tar.bz2

CC: @paulmasson @kiwifb

Component: packages: standard

Author: Ralf Stephan

Branch/Commit: 48dac6b

Reviewer: Paul Masson, Travis Scrimshaw

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

@rwst rwst added this to the sage-7.4 milestone Oct 2, 2016
@rwst

This comment has been minimized.

@rwst
Copy link
Author

rwst commented Oct 2, 2016

Branch: u/rws/upgrade_to_pynac_0_6_91

@rwst
Copy link
Author

rwst commented Oct 2, 2016

Commit: aeb81e3

@rwst
Copy link
Author

rwst commented Oct 2, 2016

New commits:

b27d68dversion/chksum
aeb81e3changes to support pynac-0.6.91

@rwst
Copy link
Author

rwst commented Oct 2, 2016

Author: Ralf Stephan

@rwst

This comment has been minimized.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Oct 2, 2016

comment:5

Documentation builds, but failing doctest in fast_callable.pyx.

Would like more information on "overflow exceptions in cot/csc", which is presumably this commit. What does this fix exactly?

The versioning is nonstandard. If this is a minor update it should be 0.6.9.1, otherwise 0.6.10.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Oct 2, 2016

Reviewer: Paul Masson

@paulmasson paulmasson mannequin added s: needs work and removed s: needs review labels Oct 2, 2016
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

9634b8a21623: revert a doctest change

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 3, 2016

Changed commit from aeb81e3 to 9634b8a

@rwst
Copy link
Author

rwst commented Oct 3, 2016

comment:7

Replying to @paulmasson:

Documentation builds, but failing doctest in fast_callable.pyx.

Curious. The doctest change seemeed necessary, but not anymore.

Would like more information on "overflow exceptions in cot/csc", which is presumably this commit. What does this fix exactly?

I think I explained this already. The only exact numeric value that can go into inverse() is zero. All others are inexact. Otherwise the doctest that fails is:

File "src/sage/functions/trig.py", line 268, in sage.functions.trig.Function_cot.__init__
Failed example:
    cot(float(0))
Exception raised:
    Traceback (most recent call last):
      File "/home/ralf/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 498, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/ralf/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 861, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.functions.trig.Function_cot.__init__[16]>", line 1, in <module>
        cot(float(Integer(0)))
      File "sage/symbolic/function.pyx", line 996, in sage.symbolic.function.BuiltinFunction.__call__ (build/cythonized/sage/symbolic/function.cpp:11008)
        res = super(BuiltinFunction, self).__call__(
      File "sage/symbolic/function.pyx", line 488, in sage.symbolic.function.Function.__call__ (build/cythonized/sage/symbolic/function.cpp:6241)
        res = g_function_eval1(self._serial,
    OverflowError: numeric::inverse(): division by zero

The versioning is nonstandard. If this is a minor update it should be 0.6.9.1, otherwise 0.6.10.

Where do you get the idea of a standard for versioning? I could use ISO 8601 of the release date as well for the version string. There is a tradition to use major/minor/micro for Pynac, and I changed micro from 9 to 91 (ninety one) to stay lexicographically consecutive.


New commits:

9634b8a21623: revert a doctest change

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Oct 3, 2016

comment:9

Replying to @rwst:

The versioning is nonstandard. If this is a minor update it should be 0.6.9.1, otherwise 0.6.10.

Where do you get the idea of a standard for versioning? I could use ISO 8601 of the release date as well for the version string. There is a tradition to use major/minor/micro for Pynac, and I changed micro from 9 to 91 (ninety one) to stay lexicographically consecutive.

The traditional split indicates the use of semantic versioning. In semantic versioning numbers are incremented numerically, not lexicographically. See item (2) in this standard: http://semver.org

@rwst
Copy link
Author

rwst commented Oct 3, 2016

comment:10

Nothing there forbids incrementing by 82.

@tscrim
Copy link
Collaborator

tscrim commented Oct 6, 2016

comment:11

Because of this change:

diff --git a/src/sage/functions/trig.py b/src/sage/functions/trig.py
index 40c0586..fba1100 100644
--- a/src/sage/functions/trig.py
+++ b/src/sage/functions/trig.py
@@ -265,6 +265,15 @@ class Function_cot(GinacFunction):
 
         TESTS:
 
+            sage: cot(float(0))
+            Infinity
+            sage: cot(SR(0))
+            Infinity
+            sage: cot(float(0.1))
+            9.966644423259238
+            sage: type(_)
+            <type 'float'>
+
         Test complex input::
 
             sage: cot(complex(1,1))     # rel tol 1e-15

The doc is incorrect. You need to change TESTS: -> TESTS::.

Other than that, I would be willing to set this to a positive review unless Paul has any objections.

@tscrim
Copy link
Collaborator

tscrim commented Oct 6, 2016

Changed reviewer from Paul Masson to Paul Masson, Travis Scrimshaw

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 6, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

48dac6b21623: doc fix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 6, 2016

Changed commit from 9634b8a to 48dac6b

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Oct 6, 2016

comment:13

I have a problem with the version number. In semantic versioning, incrementing by 82 implies that many private releases in between versions, which clearly wasn't the case. This version number is inconsistent with the entire versioning history of Pynac.

This version number appears to be an arbitrary choice. As a belatedly recognized direct contributor to this version, I think it is well within my purview to request a modification. As more of the symbolics of Sage is moved into Pynac, I would like to see that project become more of a true collaboration. Changing the version on GitHub is a straightforward process and would give evidence of acceptance of feedback.

I don't think this ticket should be approved with the current version number.

@tscrim
Copy link
Collaborator

tscrim commented Oct 6, 2016

comment:14

I think this is in the realm of bikeshedding.

@kiwifb
Copy link
Member

kiwifb commented Oct 6, 2016

comment:15

While I disapprove of the numbering, I don't care enough to make it an issue for inclusion/review.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Oct 7, 2016

comment:16

Travis, since there is no review process for Pynac this is the only place to question arbitrary decisions made by one person. If you consider that trivial, so be it.

@paulmasson
Copy link
Mannequin

paulmasson mannequin commented Oct 7, 2016

Changed reviewer from Paul Masson, Travis Scrimshaw to Travis Scrimshaw

@rwst
Copy link
Author

rwst commented Oct 7, 2016

comment:17

The Pynac review process happens here, nothing new. Maintainer decisions are often seen as arbitrary. I won't devote time to revert a numbering issue. Rather, I started yesterday on fast series expansion using Flint, which will make a new minor number change necessary because of the hard dependency. So if 0.6.91 doesn't get positive review it'll be replaced by 0.7.0 later, anyway. The numbering criticism is noted and will be followed when 0.7.10 is on the plate. Or if.

@tscrim
Copy link
Collaborator

tscrim commented Oct 7, 2016

Changed reviewer from Travis Scrimshaw to Paul Masson, Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Oct 7, 2016

comment:18

IMO, let's try to sneak this into the next Sage release. So I'm setting this to a positive review.

@rwst
Copy link
Author

rwst commented Oct 15, 2016

comment:19

Replying to @rwst:

...fast series expansion using Flint, which will make a new minor number change necessary because of the hard dependency.

It's done. See #14878.

@vbraun
Copy link
Member

vbraun commented Oct 21, 2016

Changed branch from u/rws/upgrade_to_pynac_0_6_91 to 48dac6b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants