Opened 4 years ago

Closed 4 years ago

#19312 closed enhancement (fixed)

Update to pynac-0.5.2

Reported by: rws Owned by:
Priority: major Milestone: sage-6.10
Component: packages: standard Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Jeroen Demeyer, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: bdda35e (Commits) Commit: bdda35e186887d05bd50fac9db19b785a5109e2e
Dependencies: #19606 Stopgaps:

Description (last modified by rws)

The next big Pynac update has:

  • libgmp bigints and bigrats instead of Integer and Rational Python objects
  • assumptions/domains can influence Pynac computation
  • sync elementary assumptions on symbols/functions with Pynac (#19035)
  • C++11 results in better code readability
  • better decisions through more info flags and logic
  • return not implemented and undecidable for some decisions (for math. logic changes see the #19040 description)
  • add inexact flag for possible expansions / FP simplifications
  • abs() expansion
  • function::info()
  • from GiNaC: Fix pow(+(...),2).expand()
  • remove some unused files and other cleanup; add missing autoconf macro;
  • some older memleaks closed

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

Change History (75)

comment:1 Changed 4 years ago by rws

  • Dependencies set to #19298

comment:2 Changed 4 years ago by rws

  • Description modified (diff)

Mind you, it's not here yet. Git master has half a dozen small but difficult to trace bugs.

comment:3 Changed 4 years ago by rws

  • Branch set to u/rws/19312-1

comment:4 Changed 4 years ago by rws

  • Commit set to 8421ab9a0ad240f631d434a00c6690bcb94676c3
  • Description modified (diff)
  • Status changed from new to needs_review

This works and can be reviewed independently on machines with gcc>=4.6, but please wait with setting positive for the build system changes (i.e., #19298).

There are two failing doctests: one only happens on doctesting and needs the associated environment; the other is the "pi in RIF" doctest which presented a philosophical problem to Sage developers (see #17984) and thus cannot be resolved in my lifetime.


New commits:

281541dpackage version and checksum
e56e779C++11 compile switches
474fff619312: pynac/Sage interface adaptations
5b5121519312: link assume/forget to pynac
626fb4719312: core of math logic rewrite of Expression.__nonzero__(), dedicated is_zero
9b8a09e19312: code changes in consequence of ex.__nonzero__() math logic changes
361e3d819312: doc/doctest changes in consequence of math. logic changes
8421ab919312: other doc/doctest changes because of pynac changes

comment:5 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • Milestone changed from sage-6.9 to sage-6.10

comment:6 Changed 4 years ago by rws

  • Description modified (diff)

comment:7 Changed 4 years ago by rws

  • Description modified (diff)

comment:8 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work
sage -t src/sage/plot/arc.py
**********************************************************************
File "src/sage/plot/arc.py", line 66, in sage.plot.arc.Arc.__init__
Failed example:
    bool(A[0].angle == pi/4)
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/plot/arc.py", line 70, in sage.plot.arc.Arc.__init__
Failed example:
    bool(A[0].s2 == pi)
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   2 of  12 in sage.plot.arc.Arc.__init__
    [47 tests, 2 failures, 2.28 s]
sage -t src/sage/structure/parent.pyx
**********************************************************************
File "src/sage/structure/parent.pyx", line 1227, in sage.structure.parent.Parent.__contains__
Failed example:
    pi in RIF # there is no element of RIF equal to pi
Expected:
    False
Got:
    True
**********************************************************************

comment:9 Changed 4 years ago by git

  • Commit changed from 8421ab9a0ad240f631d434a00c6690bcb94676c3 to 47842548bad14f96923220da7de6df6cab96a436

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

478425419312: fix omission, doctest

comment:10 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

comment:11 follow-up: Changed 4 years ago by rws

I agree to try first a merge of #19040 to reduce the size of this patch.

comment:12 in reply to: ↑ 11 Changed 4 years ago by jdemeyer

Replying to rws:

I agree to try first a merge of #19040 to reduce the size of this patch.

The other way around seems more logical to me: first merge this, then #19040.

comment:13 Changed 4 years ago by rws

  • Branch changed from u/rws/19312-1 to u/rws/19312-2

comment:14 Changed 4 years ago by rws

  • Commit changed from 47842548bad14f96923220da7de6df6cab96a436 to eeedddc1b81e4e9d2088283f21bac8c66476caa1

I excised the math logic changes. Please review.


New commits:

3a4844619312: package version and checksum
cc6a6c919312: C++11 compile switches
caa0aeb19312: pynac/Sage interface adaptations
a97307119312: link assume/forget to pynac
6cf8edb19312: changes in Expression.__nonzero__ due to Pynac interface change
d340d9d19312: doctest adaptations
fd68fd519312: fix code for failing doctest
eeedddc19312: kludge to allow unchanged __nonzero__ with pynac-0.5

comment:15 Changed 4 years ago by rws

  • Description modified (diff)

comment:16 follow-up: Changed 4 years ago by tscrim

FYI - There is an Unknown located in sage.misc.unknown.

Last edited 4 years ago by tscrim (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 4 years ago by rws

Replying to tscrim:

FYI - There is an Unknown located in sage.misc.unknown.

That would be useful for #19040, thanks!

comment:18 Changed 4 years ago by tscrim

  • Status changed from needs_review to needs_work

Needs a rebase.

comment:19 Changed 4 years ago by rws

If you commit to the review I'll do the rebase.

comment:20 Changed 4 years ago by tscrim

I should be able to do the review.

comment:21 Changed 4 years ago by git

  • Commit changed from eeedddc1b81e4e9d2088283f21bac8c66476caa1 to da6fe01c1e399e829124c887f9be0b75f16f6e1e

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

da6fe01Merge branch 'develop' into t/19312/19312-2

comment:22 Changed 4 years ago by git

  • Commit changed from da6fe01c1e399e829124c887f9be0b75f16f6e1e to dfb2ba3790253582ca7ec2e5eb6ce3c2aadbe798

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

dfb2ba319321:fix typo

comment:23 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

Great. So please review.

comment:24 Changed 4 years ago by jdemeyer

  • Dependencies #19298 deleted

comment:25 follow-up: Changed 4 years ago by jdemeyer

  • Dependencies set to #19606

This should be rebased on top of #19606.

comment:26 Changed 4 years ago by jdemeyer

Replace

if not (solution_dict == True or solution_dict == False)

by

if solution_dict is not True and solution_dict is not False

comment:27 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Why do you need the __cmp__ method on Constant? Since we are trying to move to Python 3, we shouldn't add new __cmp__ or cmp().

comment:28 Changed 4 years ago by jdemeyer

In src/sage/calculus/desolvers.py, I think you need to justify the changes

@@ -676,7 +676,7 @@ def desolve_laplace(de, dvar, ics=None, ivar=None):
         dvar, ivar = dvar
     elif ivar is None:
         ivars = de.variables()
-        ivars = [t for t in ivars if t != dvar]
+        ivars = [t for t in ivars if t is not dvar]
         if len(ivars) != 1:
             raise ValueError("Unable to determine independent variable, please specify.")
         ivar = ivars[0]
@@ -1182,7 +1181,7 @@ def desolve_rk4(de, dvar, ics=None, ivar=None, end_points=None, step=0.1, output
 
     if ivar is None:
         ivars = de.variables()
-        ivars = [t for t in ivars if t != dvar]
+        ivars = [t for t in ivars if repr(t) != repr(dvar)]
         if len(ivars) != 1:
             raise ValueError("Unable to determine independent variable, please specify.")
         ivar = ivars[0]

comment:29 Changed 4 years ago by jdemeyer

Replace

cdef object py_integer_from_long(long x) except +:
    cdef Integer z = PY_NEW(Integer)
    mpz_set_si(z.value, x)
    return z

by

from sage.rings.integer cimport smallInteger

cdef py_integer_from_long(long x):
    return smallInteger(x)
Last edited 4 years ago by jdemeyer (previous) (diff)

comment:30 Changed 4 years ago by jdemeyer

I also don't understand the following doctest changes:

@@ -1148,7 +1148,8 @@ def desolve_rk4(de, dvar, ics=None, ivar=None, end_points=None, step=0.1, output
 
     Variant 2 for input - more common in numerics::
 
-        sage: x,y=var('x y')
+        sage: _ = var('x,y')
+        sage: f = function('f')(x)
         sage: desolve_rk4(x*y*(2-y),y,ics=[0,1],end_points=1,step=0.5)
         [[0, 1], [0.5, 1.12419127424558], [1.0, 1.461590162288825]]
 
@@ -1156,15 +1157,13 @@ def desolve_rk4(de, dvar, ics=None, ivar=None, end_points=None, step=0.1, output
     desolve function In this example we integrate bakwards, since
     ``end_points < ics[0]``::
 
-        sage: y=function('y')(x)
-        sage: desolve_rk4(diff(y,x)+y*(y-1) == x-2,y,ics=[1,1],step=0.5, end_points=0)
+        sage: desolve_rk4(diff(f,x)+f*(f-1) == x-2,f,ics=[1,1],step=0.5, end_points=0)
         [[0.0, 8.904257108962112], [0.5, 1.909327945361535], [1, 1]]
 
     Here we show how to plot simple pictures. For more advanced
     aplications use list_plot instead. To see the resulting picture
     use ``show(P)`` in Sage notebook. ::
 
-        sage: x,y=var('x y')
         sage: P=desolve_rk4(y*(2-y),y,ics=[0,.1],ivar=x,output='slope_field',end_points=[-4,6],thickness=3)
 
     ALGORITHM:
Last edited 4 years ago by jdemeyer (previous) (diff)

comment:31 Changed 4 years ago by jdemeyer

There are various doctest failures:

sage -t --long src/sage/combinat/finite_state_machine.py
**********************************************************************
File "src/sage/combinat/finite_state_machine.py", line 10001, in sage.combinat.finite_state_machine.FiniteStateMachine.?
Failed example:
    R.<phi> = NumberField(x^2-x-1, embedding=golden_ratio)
Exception raised:
    Traceback (most recent call last):
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.finite_state_machine.FiniteStateMachine.?[11]>", line 1, in <module>
        R = NumberField(x**Integer(2)-x-Integer(1), embedding=golden_ratio, names=('phi',)); (phi,) = R._first_ngens(1)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/number_field/number_field.py", line 490, in NumberField
        return NumberField_version2(polynomial=polynomial, name=name, check=check, embedding=embedding, latex_name=latex_name, assume_disc_small=assume_disc_small, maximize_at_primes=maximize_at_primes, structure=structure)
      File "sage/structure/factory.pyx", line 366, in sage.structure.factory.UniqueFactory.__call__ (build/cythonized/sage/structure/factory.c:1328)
        return self.get_object(version, key, kwds)
      File "sage/structure/factory.pyx", line 410, in sage.structure.factory.UniqueFactory.get_object (build/cythonized/sage/structure/factory.c:1734)
        obj = self.create_object(version, key, **extra_args)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/number_field/number_field.py", line 612, in create_object
        return NumberField_quadratic(polynomial, name, latex_name, check, embedding, assume_disc_small=assume_disc_small, maximize_at_primes=maximize_at_primes, structure=structure)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/number_field/number_field.py", line 10070, in __init__
        assume_disc_small=assume_disc_small, maximize_at_primes=maximize_at_primes, structure=structure)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/number_field/number_field.py", line 6421, in __init__
        assume_disc_small=assume_disc_small, maximize_at_primes=maximize_at_primes, structure=structure)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/rings/number_field/number_field.py", line 1202, in __init__
        embedding = number_field_morphisms.create_embedding_from_approx(self, embedding)
      File "sage/rings/number_field/number_field_morphisms.pyx", line 503, in sage.rings.number_field.number_field_morphisms.create_embedding_from_approx (build/cythonized/sage/rings/number_field/number_field_morphisms.c:7328)
        raise ValueError, "%s is not a root of the defining polynomial of %s" % (gen_image, K)
    ValueError: golden_ratio is not a root of the defining polynomial of Number Field in phi with defining polynomial x^2 - x - 1
**********************************************************************
File "src/sage/combinat/finite_state_machine.py", line 10002, in sage.combinat.finite_state_machine.FiniteStateMachine.?
Failed example:
    N = NAFp.number_of_words(base_ring=R); N
Expected:
    1/5*(3*golden_ratio + 1)*golden_ratio^n
    - 1/5*(3*golden_ratio - 4)*(-golden_ratio + 1)^n
Got:
    1/10*(1/2*sqrt(5) + 1/2)^n*(3*sqrt(5) + 5) - 1/10*(-1/2*sqrt(5) + 1/2)^n*(3*sqrt(5) - 5)
**********************************************************************
File "src/sage/functions/other.py", line 1693, in sage.functions.other.Function_beta.__init__
Failed example:
    beta(1/3, 1/2)
Expected:
    beta(1/2, 1/3)
Got:
    beta(1/3, 1/2)
**********************************************************************

comment:32 follow-up: Changed 4 years ago by jdemeyer

Why do you use except + for non-extern functions? For example

cdef bint py_is_exact(object x) except +:
    return isinstance(x, int) or isinstance(x, long) or isinstance(x, Integer) or \
           (isinstance(x, Element) and
            ((<Element>x)._parent.is_exact() or (<Element>x)._parent == ring.SR))

comment:33 in reply to: ↑ 32 ; follow-up: Changed 4 years ago by rws

Replying to jdemeyer:

Why do you use except + for non-extern functions? For example

cdef bint py_is_exact(object x) except +:
    return isinstance(x, int) or isinstance(x, long) or isinstance(x, Integer) or \
           (isinstance(x, Element) and
            ((<Element>x)._parent.is_exact() or (<Element>x)._parent == ring.SR))

I have just copied from py_is_even which is not extern either and written by yourself.

comment:34 in reply to: ↑ 33 Changed 4 years ago by jdemeyer

Replying to rws:

I have just copied from py_is_even which is not extern either

Well, then py_is_even is wrong too. But ok, you can leave the except + for now, it's not a major problem.

and written by yourself.

No way. I may have been the last person to edit that line, but I certainly didn't write that function or the except +.

comment:35 Changed 4 years ago by git

  • Commit changed from dfb2ba3790253582ca7ec2e5eb6ce3c2aadbe798 to 76733e4c003da3506ac1cc15bea61b7ca147de93

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

76733e419312: address reviewer's comments

comment:36 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

comment:37 follow-up: Changed 4 years ago by tscrim

Here are my current comments:

  • I also don't understand the changes on comment:30.
  • decl_assume and decl_forget aren't doctested.
  • raise TypeError('Cannot compare: ' + repr(self)): I'm thinking that should be a ValueError since it depends on the value of the value of the expression. Also, to better conform with python, error messages are not sentences and should not start with a capital letter.
  • Could we change:
    -        if solution_dict is not True and solution_dict is not False:
    +        if not isinstance(solution_dict, bool):
    
  • I don't understand this change:
    • src/sage/geometry/hyperbolic_space/hyperbolic_isometry.py

      diff --git a/src/sage/geometry/hyperbolic_space/hyperbolic_isometry.py b/src/sage/geometry/hyperbolic_space/hyperbolic_isometry.py
      index 4840c05..f205b9c 100644
      a b def mobius_transform(A, z): 
      10541054        TypeError: A must be an invertible 2x2 matrix over the complex numbers or a symbolic ring
      10551055
      10561056    The matrix can be symbolic or can be a matrix over the real
      1057     or complex numbers, but must be invertible::
      1058 
      1059         sage: (a,b,c,d) = var('a,b,c,d');
      1060         sage: mobius_transform(matrix(2,[a,b,c,d]),I)
      1061         (I*a + b)/(I*c + d)
       1057    or complex numbers, but must be provably invertible::
      10621058
       1059        sage: (b,c) = var('b,c');
       1060        sage: mobius_transform(matrix(2,[1,b,c,b*c+1]),I)
       1061        (b + I)/(b*c + I*c + 1)
      10631062        sage: mobius_transform(matrix(2,[0,0,0,0]),I)
      10641063        Traceback (most recent call last):
      10651064        ...
    I feel it obfuscates what the Möbius transform does.

comment:38 Changed 4 years ago by jdemeyer

In src/sage/rings/number_field/number_field_morphisms.pyx, can you import test_relation_maxima only when you need it?

This doctest loses its meaning now:

  • src/sage/functions/other.py

    diff --git a/src/sage/functions/other.py b/src/sage/functions/other.py
    index 7fafc33..5769ce7 100644
    a b class Function_beta(GinacFunction): 
    16911691        Ginac might reorder the arguments::
    16921692
    16931693            sage: beta(1/3, 1/2)
    1694             beta(1/2, 1/3)
     1694            beta(1/3, 1/2)
    16951695
    16961696        The result is symbolic if exact input is given::
    16971697

comment:39 follow-up: Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This is actually a bug:

  • src/sage/symbolic/constants_c.pyx

    diff --git a/src/sage/symbolic/constants_c.pyx b/src/sage/symbolic/constants_c.pyx
    index 340d20d..fb43864 100644
    a b cdef class E(Expression): 
    217217            sage: t = e^a; t
    218218            e^a
    219219            sage: t^b
    220             (e^a)^b
     220            e^a^b
    221221            sage: SR(1).exp()
    222222            e
    223223

Usually, e^a^b is interpreted as e^(a^b):

sage: var('a,b')
(a, b)
sage: (e^a)^b
e^a^b
sage: e^a^b
e^(a^b)

comment:40 in reply to: ↑ 39 Changed 4 years ago by rws

Replying to jdemeyer:

Usually, e^a^b is interpreted as e^(a^b):

Oh, that is serious.

comment:41 follow-up: Changed 4 years ago by jdemeyer

For __hash__, could you also test a number which has a hash more than 2^32, in order to test for #19310:

sage: hash(1e30)
-442393380            # 32-bit
5076964209140210896   # 64-bit
sage: hash(SR(1e30))  # known bug #19310
-442393380            # 32-bit
5076964209140210896   # 64-bit

comment:42 in reply to: ↑ 37 ; follow-up: Changed 4 years ago by rws

Replying to tscrim:

  • I also don't understand the changes on comment:30.

They are reverted now. In the first branch of this ticket they were necessary.

  • I don't understand this change:

@@ -1054,12 +1054,11 @@ def mobius_transform(A, z):

I feel it obfuscates what the Möbius transform does.

Well you can't give a transform of a noninvertible matrix, so I changed the example to one that is always invertible. I can revert to the original doctest but it will lead to problems in a later ticket.

comment:43 in reply to: ↑ 41 Changed 4 years ago by rws

Replying to jdemeyer:

For __hash__, could you also test a number which has a hash more than 2^32, in order to test for #19310:

That doesn't work in 0.5 but some work is already done. See https://github.com/pynac/pynac/issues/95

comment:44 in reply to: ↑ 42 ; follow-up: Changed 4 years ago by tscrim

Replying to rws:

Replying to tscrim:

  • I don't understand this change:

@@ -1054,12 +1054,11 @@ def mobius_transform(A, z):

I feel it obfuscates what the Möbius transform does.

Well you can't give a transform of a noninvertible matrix, so I changed the example to one that is always invertible. I can revert to the original doctest but it will lead to problems in a later ticket.

Unless a*d - b*c == 0, then this is an invertible matrix, and this is a "rare" event for generic a,b,c,d. So I would say this is a safe test, or perhaps we can add an assumption that a*d - b*c != 0? Also could you tell me what ticket you are referring to or give some more details?

comment:45 in reply to: ↑ 44 ; follow-up: Changed 4 years ago by rws

Replying to tscrim:

Replying to rws:

Well you can't give a transform of a noninvertible matrix, so I changed the example to one that is always invertible. I can revert to the original doctest but it will lead to problems in a later ticket.

Unless a*d - b*c == 0, then this is an invertible matrix, and this is a "rare" event for generic a,b,c,d. So I would say this is a safe test, or perhaps we can add an assumption that a*d - b*c != 0? Also could you tell me what ticket you are referring to or give some more details?

If I remember correctly there was a problem due to the Expression.__nonzero__ changes in the first branch. So this might resurface with #19040. I'm sorry I can't give more details, let's just see about it whenever #19040 gets tackled.

comment:46 Changed 4 years ago by git

  • Commit changed from 76733e4c003da3506ac1cc15bea61b7ca147de93 to 976c80e1a04a58a50d70714da88f6df064952c38

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

976c80e19312: address reviewer's comments

comment:47 in reply to: ↑ 25 Changed 4 years ago by rws

This should cover everything except the exponentiation bug and...

Replying to jdemeyer:

This should be rebased on top of #19606.

As I need to make a new release, and #19606 will be in the next beta I'll just do a rebase later.

comment:48 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer, Travis Scrimshaw

comment:49 Changed 4 years ago by git

  • Commit changed from 976c80e1a04a58a50d70714da88f6df064952c38 to c5f20d691e9f9023223f92f2c7e47ed19b1dc2a4

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

c9e4c7e19312: build dir changes for version 0.5.1
c5f20d619312: doctest and some code fixes with pynac-0.5.1

comment:50 Changed 4 years ago by rws

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Summary changed from Update to pynac-0.5.0 to Update to pynac-0.5.1

pynac-0.5.1 fixed the exp bug (and contains a previous fix). This should be it.

comment:51 in reply to: ↑ 45 Changed 4 years ago by tscrim

Replying to rws:

If I remember correctly there was a problem due to the Expression.__nonzero__ changes in the first branch. So this might resurface with #19040. I'm sorry I can't give more details, let's just see about it whenever #19040 gets tackled.

I will be happy to work on it if there is a problem with #19040.

I'm guessing changes of this type:

-        sage: x,y = var('x,y')
+        sage: _ = var('x,y')

are because you're thinking towards #17958. However I don't think that takes away from the problem, but instead just makes the behavior harder to do find/replace for and makes it harder for the reader to understand what is going on in the doctest. Could you provide some other justification for these changes?

comment:52 Changed 4 years ago by git

  • Commit changed from c5f20d691e9f9023223f92f2c7e47ed19b1dc2a4 to 065791ecfa672c2dec1752fd16a473d776625775

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

065791e19312: address reviewer's comments

comment:53 Changed 4 years ago by rws

Replying to tscrim:

I'm guessing changes of this type:

-        sage: x,y = var('x,y')
+        sage: _ = var('x,y')

are because you're thinking towards #17958. However I don't think that takes away from the problem, but instead just makes the behavior harder to do find/replace for and makes it harder for the reader to understand what is going on in the doctest. Could you provide some other justification for these changes?

If you object to that you will have to stem the tide now by posting to sage-devel, because a recursive grep just showed about 30 instances of such usage in Sage (and certainly not all from me). Or open a separate ticket, please. Here they are now reverted.

comment:54 Changed 4 years ago by jdemeyer

This

isinstance(rel, bool) and rel is True

can be simplified to

rel is True

comment:55 Changed 4 years ago by jdemeyer

Why this?

  • src/sage/symbolic/constants.py

    diff --git a/src/sage/symbolic/constants.py b/src/sage/symbolic/constants.py
    index 2b4d533..838f64f 100644
    a b class Constant(object): 
    303303            True
    304304            sage: p == s
    305305            False
    306             sage: p != s
    307             True
    308306        """
    309307        return (self.__class__ == other.__class__ and
    310308                self._name == other._name)

comment:56 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This is not valid LaTeX:

e^{\sqrt{x}}^{x}

comment:57 Changed 4 years ago by git

  • Commit changed from 065791ecfa672c2dec1752fd16a473d776625775 to af71087f3627150731897d466fa395c7f661d290

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

d7e7981build dir changes for 0.5.2
af71087address reviewer's comments; add doctests

comment:58 Changed 4 years ago by rws

  • Description modified (diff)
  • Status changed from needs_work to needs_review

This is not valid LaTeX:

Is changed. The doctest removal also was to satisfy the first branch. I have reincluded it. Another possible matter with #19040.

comment:59 Changed 4 years ago by rws

  • Summary changed from Update to pynac-0.5.1 to Update to pynac-0.5.2

comment:60 Changed 4 years ago by vdelecroix

What is the definition of an "undecidable relation"? (see also the comments: comment:24 and comment:41 on #19040).

comment:61 Changed 4 years ago by rws

Please see my reply there.

comment:62 Changed 4 years ago by git

  • Commit changed from af71087f3627150731897d466fa395c7f661d290 to 6c27be1795b55e170f3d8e927dfebace34b39abd

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

6c27be1Merge branch 'develop' into t/19312/19312-2

comment:63 follow-up: Changed 4 years ago by tscrim

There still is

e^{\sqrt{x}}^{x}

in functions/log.py.

Also, for the test in hyperbolic_isometry.py, if we add

sage: assume(a*d != b*c)

then would that avoid issues with #19040?

Other than that, I'm okay with the current state up to Jeroen's comments that haven't been addressed.

comment:64 Changed 4 years ago by git

  • Commit changed from 6c27be1795b55e170f3d8e927dfebace34b39abd to 7722756efa80d790e57f59a5b2010f82005b4a3f

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

772275619312: fix doctest

comment:65 in reply to: ↑ 63 Changed 4 years ago by rws

Replying to tscrim:

Also, for the test in hyperbolic_isometry.py, if we add

sage: assume(a*d != b*c)

then would that avoid issues with #19040?

I'm unable to answer this at the moment but I appreciate the idea. Thanks for your review.

comment:66 Changed 4 years ago by jdemeyer

Regarding #19606: Is pynac now linked with gmp? If so, you need to revert #19606 and you need to change the dependencies of the package.

I have no other comments (which doesn't mean that I understand every change you made, but at least I don't see any obvious problems).

comment:67 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_info

comment:68 Changed 4 years ago by git

  • Commit changed from 7722756efa80d790e57f59a5b2010f82005b4a3f to f71e3d8f9579f0b0b81f6dae477c3d61bcad689d

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

f71e3d819312: add new pynac dependencies

comment:69 Changed 4 years ago by rws

  • Status changed from needs_info to needs_review

Indeed, thanks.

comment:70 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

I think then we are good to go.

comment:71 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

32-bit fails with

sage -t --long src/sage/symbolic/expression.pyx
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 1455, in sage.symbolic.expression.Expression.__hash__
Failed example:
    hash(SR(19.23))
Expected:
    2836855582
Got:
    -1458111714
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 1457, in sage.symbolic.expression.Expression.__hash__
Failed example:
    hash(19.23)
Expected:
    2836855582
Got:
    -1458111714
**********************************************************************
1 item had failures:
   2 of  26 in sage.symbolic.expression.Expression.__hash__

comment:72 Changed 4 years ago by git

  • Commit changed from f71e3d8f9579f0b0b81f6dae477c3d61bcad689d to bdda35e186887d05bd50fac9db19b785a5109e2e

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

bdda35e19312: differentiate doctest result wrt 32/64 bit

comment:73 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

comment:74 Changed 4 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:75 Changed 4 years ago by vbraun

  • Branch changed from u/rws/19312-2 to bdda35e186887d05bd50fac9db19b785a5109e2e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.