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.7.10 #23325

Closed
rwst opened this issue Jun 26, 2017 · 60 comments
Closed

Upgrade to Pynac-0.7.10 #23325

rwst opened this issue Jun 26, 2017 · 60 comments

Comments

@rwst
Copy link
Contributor

rwst commented Jun 26, 2017

In the new version:

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

Depends on #23329

CC: @kiwifb

Component: packages: standard

Author: Ralf Stephan

Branch/Commit: 5387ba5

Reviewer: Jeroen Demeyer

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

@rwst rwst added this to the sage-8.1 milestone Jun 26, 2017
@rwst

This comment has been minimized.

@rwst
Copy link
Contributor Author

rwst commented Jun 26, 2017

Branch: u/rws/upgrade_to_pynac_0_7_9

@rwst
Copy link
Contributor Author

rwst commented Jun 26, 2017

New commits:

1d1aeb823325: pkg version/chksum, remove obsolete patch
cf6218023325: fix py_get_parent_char()
f976b8c23325: fix doctests

@rwst
Copy link
Contributor Author

rwst commented Jun 26, 2017

Author: Ralf Stephan

@rwst

This comment has been minimized.

@rwst
Copy link
Contributor Author

rwst commented Jun 26, 2017

Commit: f976b8c

@jdemeyer
Copy link
Contributor

comment:4

I don't agree with the change to py_get_parent_char: why assume characteristic zero if the parent cannot decide?

@jdemeyer
Copy link
Contributor

Reviewer: Jeroen Demeyer

@jdemeyer
Copy link
Contributor

comment:5

If you revert the change to py_get_parent_char and all doctests pass, you can set this to positive review.

@rwst
Copy link
Contributor Author

rwst commented Jun 27, 2017

comment:6

That's a no, because this then needs investigation (doctest fail at src/sage/manifolds/differentiable/diff_form.py:796):

sage: M = Manifold(3, 'R3', '\RR^3', start_index=1)
sage: c_cart.<x,y,z> = M.chart()
sage: eps = M.diff_form(3, 'epsilon', r'\epsilon')
sage: eps[1,2,3] = 1
sage: a = M.one_form('A')
sage: a[:] = (x*y*z, -z*x, y*z)
sage: b = M.one_form('B')
sage: b[:] = (cos(z), sin(x), cos(y))
sage: ab = a.wedge(b)
sage: dab = ab.exterior_derivative()
sage: dab[[1,2,3]]/eps[[1,2,3]]
...
/home/ralf/sage/src/sage/structure/element.pyx in sage.structure.element.RingElement._div_ (build/cythonized/sage/structure/element.c:17655)()
   2453         return l
   2454 
-> 2455     cpdef _div_(self, other):
   2456         """
   2457         Default implementation of division using the fraction field.

/home/ralf/sage/local/lib/python2.7/site-packages/sage/manifolds/coord_func_symb.pyc in _div_(self, other)
    947         if other._express.is_zero():
    948             raise ZeroDivisionError("division of a coordinate function by zero")
--> 949         res = self._simplify(self._express / SR(other))
    950         if res.is_trivial_zero():  # NB: "if res == 0" would be too
    951                                    # expensive (cf. #22859)

/home/ralf/sage/src/sage/structure/element.pyx in sage.structure.element.Element.__div__ (build/cythonized/sage/structure/element.c:12950)()
   1638         cdef int cl = classify_elements(left, right)
   1639         if HAVE_SAME_PARENT(cl):
-> 1640             return (<Element>left)._div_(right)
   1641         if BOTH_ARE_ELEMENT(cl):
   1642             return coercion_model.bin_op(left, right, div)

/home/ralf/sage/src/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression._div_ (build/cythonized/sage/symbolic/expression.cpp:24795)()
   3445                 raise ZeroDivisionError("Symbolic division by zero")
   3446             else:
-> 3447                 raise
   3448         finally:
   3449             sig_off()

/home/ralf/sage/src/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression._div_ (build/cythonized/sage/symbolic/expression.cpp:24684)()
   3437                                o)
   3438             else:
-> 3439                 x = left._gobj / _right._gobj
   3440             return new_Expression_from_GEx(left._parent, x)
   3441         except Exception as msg:

/home/ralf/sage/src/sage/libs/pynac/pynac.pyx in sage.libs.pynac.pynac.py_get_parent_char (build/cythonized/sage/libs/pynac/pynac.cpp:9942)()
    818         return 0
    819 
--> 820     c = (<Element>o)._parent.characteristic()
    821 
    822     # Pynac only differentiates between

/home/ralf/sage/local/lib/python2.7/site-packages/sage/categories/rings.pyc in characteristic(self)
    461             from sage.rings.infinity import infinity
    462             from sage.rings.integer_ring import ZZ
--> 463             order_1 = self.one().additive_order()
    464             return ZZ.zero() if order_1 is infinity else order_1
    465 

/home/ralf/sage/src/sage/structure/element.pyx in sage.structure.element.RingElement.additive_order (build/cythonized/sage/structure/element.c:18049)()
   2470         Return the additive order of ``self``.
   2471         """
-> 2472         raise NotImplementedError
   2473 
   2474     def multiplicative_order(self):

NotImplementedError: 

sage: dab[[1,2,3]]
Scalar field on the 3-dimensional differentiable manifold R3
sage: type(_)
<class 'sage.manifolds.differentiable.scalarfield_algebra.DiffScalarFieldAlgebra_with_category.element_class'>
sage: eps[[1,2,3]]
Scalar field on the 3-dimensional differentiable manifold R3
sage: type(_)
<class 'sage.manifolds.differentiable.scalarfield_algebra.DiffScalarFieldAlgebra_with_category.element_class'>

sage: parent(dab[[1,2,3]])
Algebra of differentiable scalar fields on the 3-dimensional differentiable manifold R3
sage: parent(eps[[1,2,3]])
Algebra of differentiable scalar fields on the 3-dimensional differentiable manifold R3

While this is triggered by changes in Pynac that should be addressed it's not clear to me why the division of two scalar fields must go through SR at all.

@jdemeyer
Copy link
Contributor

comment:7

I will have a look.

@rwst
Copy link
Contributor Author

rwst commented Jun 27, 2017

comment:8

The reason why this is now failing is that the quotient here, which is 1 but not integer, is now recognized as is_one and triggers the ...pos_char call.

https://github.com/pynac/pynac/blob/master/ginac/mul.cpp#L702

Removing the call altogether in preliminary testing only fails one doctest involving Mod and, frankly, I could care less. Full testing now.

@jdemeyer
Copy link
Contributor

comment:9

I think the best solution here is simply to implement characteric for the "Ring of coordinate functions".

@jdemeyer
Copy link
Contributor

Dependencies: #23329

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2017

Changed commit from f976b8c to fb52b8b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2017

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

fb52b8b23325: revert libs/pynac commit

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2017

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

1a369cbMerge branch 'develop' into t/23325/upgrade_to_pynac_0_7_9
1fa0f69Merge branch 'develop' into t/23325/upgrade_to_pynac_0_7_9
725db22Merge branch 'develop' into t/23325/upgrade_to_pynac_0_7_9
3b7db8bpynac-2.7.10 doctest fixes
f5682bfnecessary code change to make pynac-2.7.10 work

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 4, 2017

Changed commit from fb52b8b to f5682bf

@rwst
Copy link
Contributor Author

rwst commented Aug 4, 2017

comment:13

Correct is 0.7.10 not 2.7.10.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2017

Changed commit from f5682bf to 0879761

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2017

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

bafd41aMerge branch 'develop' into t/23325/upgrade_to_pynac_0_7_9
a90ce6a23582: Robustify doctest in hyperbolic_geodesic.py
0879761Merge branch 'u/rws/robustify_doctest_in_hyperbolic_geodesic_py' of git://trac.sagemath.org/sage into t/23325/upgrade_to_pynac_0_7_9

@rwst
Copy link
Contributor Author

rwst commented Aug 5, 2017

Changed dependencies from #23329 to #23329, #23582

@rwst
Copy link
Contributor Author

rwst commented Aug 6, 2017

Changed dependencies from #23329, #23582 to #23329, #23582, #23587

@jdemeyer
Copy link
Contributor

New commits:

b54f51bDoc formatting

@jdemeyer
Copy link
Contributor

Changed commit from 62cebba to b54f51b

@vbraun
Copy link
Member

vbraun commented Aug 13, 2017

Changed branch from u/jdemeyer/23325-1 to b54f51b

@vbraun
Copy link
Member

vbraun commented Aug 13, 2017

comment:37

Fails on 32-bit Linux

**********************************************************************
File "src/doc/de/thematische_anleitungen/sage_gymnasium.rst", line 191, in doc.de.thematische_anleitungen.sage_gymnasium
Failed example:
    sqrt(18)
Expected:
    3*sqrt(2)
Got:
    sqrt(18)
**********************************************************************
1 item had failures:
   1 of 294 in doc.de.thematische_anleitungen.sage_gymnasium
    [207 tests, 1 failure, 5.39 s]
**********************************************************************
File "src/sage/functions/orthogonal_polys.py", line 1668, in sage.functions.orthogonal_polys.Func_assoc_legendre_Q._eval_
Failed example:
    gen_legendre_Q(2,1,3)
Expected:
    -1/4*sqrt(-2)*(-36*I*pi + 36*log(4) - 36*log(2) - 25)
Got:
    -1/8*sqrt(-8)*(-36*I*pi + 36*log(4) - 36*log(2) - 25)
**********************************************************************
1 item had failures:
   1 of   2 in sage.functions.orthogonal_polys.Func_assoc_legendre_Q._eval_
    [378 tests, 1 failure, 7.79 s]
**********************************************************************
File "src/sage/rings/number_field/number_field_base.pyx", line 219, in sage.rings.number_field.number_field_base.NumberField.minkowski_bound
Failed example:
    B = K.minkowski_bound(); B
Expected:
    16/3*sqrt(3)/pi
Got:
    8/9*sqrt(108)/pi
**********************************************************************
File "src/sage/rings/number_field/number_field_base.pyx", line 230, in sage.rings.number_field.number_field_base.NumberField.minkowski_bound
Failed example:
    B = K.minkowski_bound(); B
Expected:
    sqrt(10)
Got:
    1/2*sqrt(40)
**********************************************************************
1 item had failures:
   2 of  19 in sage.rings.number_field.number_field_base.NumberField.minkowski_bound
    [74 tests, 2 failures, 2.09 s]
**********************************************************************
File "src/sage/rings/real_mpfr.pyx", line 3961, in sage.rings.real_mpfr.RealNumber.sqrt
Failed example:
    r.sqrt()
Expected:
    2*sqrt(1086)
Got:
    sqrt(4344)
**********************************************************************
1 item had failures:
   1 of  13 in sage.rings.real_mpfr.RealNumber.sqrt
    [1000 tests, 1 failure, 3.14 s]

@vbraun
Copy link
Member

vbraun commented Aug 13, 2017

Changed commit from b54f51b to none

@vbraun vbraun reopened this Aug 13, 2017
@rwst
Copy link
Contributor Author

rwst commented Aug 16, 2017

comment:38

Okay, I'm installing virtualization (a first) and Ubuntu 32-bit to get the ability to test on 32-bit.

@rwst
Copy link
Contributor Author

rwst commented Aug 19, 2017

Changed branch from b54f51b to u/rws/b54f51bab33a602448c1b3e2f719fb375661aac3

@rwst
Copy link
Contributor Author

rwst commented Aug 19, 2017

Changed branch from u/rws/b54f51bab33a602448c1b3e2f719fb375661aac3 to b54f51b

@rwst
Copy link
Contributor Author

rwst commented Aug 19, 2017

comment:41

STDERR: error: Server does not allow request for unadvertised object b54f51bab33a602448c1b3e2f719fb375661aac3

So I'll push to my previous branch u/rws/23325-1 (including Jeroen's fix) in the hope you can pick up its last commit, which contains a last-minute fix.

@rwst
Copy link
Contributor Author

rwst commented Aug 19, 2017

Changed branch from b54f51b to u/rws/23325-1

@rwst
Copy link
Contributor Author

rwst commented Aug 19, 2017

New commits:

a887a8b23325: chksum,version
98bab3723325: remove obsolete patch
fe100f723325: explicitly look for sage-python23
8fe77a723325: code change to make use of new pynac
62cebba23325: doctest fixes
5a2da5823325: doc formatting
5387ba523325: last-minute 32-bit fix

@rwst
Copy link
Contributor Author

rwst commented Aug 19, 2017

Commit: 5387ba5

@kiwifb
Copy link
Member

kiwifb commented Aug 22, 2017

comment:44

I think it can just be put back to positive review. Especially if the last fix is the stuff for 32bits problems.

@vbraun
Copy link
Member

vbraun commented Aug 29, 2017

Changed branch from u/rws/23325-1 to 5387ba5

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

5 participants