Opened 5 years ago

Closed 5 years ago

#19819 closed enhancement (fixed)

Update to pynac-0.6.0

Reported by: rws Owned by:
Priority: major Milestone: sage-7.0
Component: packages: standard Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Jeroen Demeyer, Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 33a4faa (Commits, GitHub, GitLab) Commit: 33a4faab1ff70d2d3b2f0d3f61248a61bae57149
Dependencies: Stopgaps:

Status badges

Description (last modified by rws)

Changes vs 0.5.3:

  • fix expansion of abs(power) (#19797)
  • fix a wrong function option with zeta/zeta2 (#19799)
  • fix wrong formula for tan(...).imag() (#19791)
  • fix mpq_pythonhash (#19310)
  • change hash type to long (#19310)
  • expand exp(m/n*pi*I) with trig functions if they can be expanded (#19738)
  • start changing libtool version with each release
  • performance: in exp_eval check for Pi and I before trying to divide
  • performance: much less canonicalizations of GMP rationals
  • performance: cache pseries coeff accesses in pseries::mul_series
  • performance: GinacFunctions for cot/acot
  • C++11: add override where possible

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

Change History (58)

comment:1 in reply to: ↑ description ; follow-up: Changed 5 years ago by jdemeyer

Replying to rws:

  • change hash type to int64_t (#19310)

Why int64_t? The type should be long.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 5 years ago by fbissey

Replying to jdemeyer:

Replying to rws:

  • change hash type to int64_t (#19310)

Why int64_t? The type should be long.

long doesn't guarantee anything in term of width. If you want something that is 64bit wide, you better be explicit about it. The C standard is rather annoying when it comes to variable size, especially long types - unless I missed something in a newer version of the standard.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 5 years ago by jdemeyer

Replying to fbissey:

If you want something that is 64bit wide

We don't.

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

Replying to jdemeyer:

Replying to fbissey:

If you want something that is 64bit wide

We don't.

You will get doctest fails (#19310) on 32-bit.

comment:5 follow-up: Changed 5 years ago by rws

And if you don't care about 32bit: int64_t is typedefed to long on 64bit. I won't rewrite this just because you don't like the look of int64_t.

comment:6 in reply to: ↑ 5 Changed 5 years ago by jdemeyer

Replying to rws:

And if you don't care about 32bit

I care about 32 bit and about 64 bit.

I won't rewrite this just because you don't like the look of int64_t.

I don't think you need to rewrite anything. A simple search-and-replace int64_t -> long would probably work.

comment:7 in reply to: ↑ 4 Changed 5 years ago by jdemeyer

Replying to rws:

You will get doctest fails (#19310) on 32-bit.

I don't understand this, can you elaborate?

comment:8 follow-ups: Changed 5 years ago by rws

sage: hash(SR(2^32))
0

This will be the result on 32bit if I use long and so pickles won't be usable on 64bit.

comment:9 in reply to: ↑ 8 Changed 5 years ago by jdemeyer

Replying to rws:

so pickles won't be usable on 64bit.

What has hashing to do with pickling?

comment:10 in reply to: ↑ 8 Changed 5 years ago by jdemeyer

Replying to rws:

sage: hash(SR(2^32))
0

This will be the result on 32bit if I use long

That's also what the result should be on 32 bit. Are you implying that this is somehow wrong?

comment:11 Changed 5 years ago by rws

  • Branch set to u/rws/19819-1

comment:12 Changed 5 years ago by rws

  • Commit set to ea6a8fac59921b69c8e9caede213a697064e5494

I always love this. So that corrects some misunderstanding with me, many thanks. Before I do the next release could you please have a look at the branch? I'm sure I have made more mistakes.


New commits:

ea6a8faproposed changes because of pynac-0.5.4

comment:13 Changed 5 years ago by jdemeyer

This is certainly right:

            sage: hash(SR(19/23)) == hash(19/23)
            True
            sage: hash(SR(2^32)) == hash(ZZ(2^32))
            True

and it's exactly the reason why you want hash(SR(2^32)) == 0 on 32-bit.

comment:14 Changed 5 years ago by jdemeyer

I don't really understand the doctest changes of the form

-        sage: QQbar(e^(pi*I/3))
+        sage: QQbar(exp(pi*I/3, hold=True))

comment:15 follow-up: Changed 5 years ago by jdemeyer

Nor this one:

-        sage: N(integrate(sin(x^2)/(x^2), x, 1, infinity))
-        0.285736646322853
+        sage: N(integrate(sin(x^2)/(x^2), x, 1, infinity), prec=40)
+        0.28573664632

comment:16 Changed 5 years ago by jdemeyer

I would keep this doctest and mark it as # not implemented (it's a case where the original simplification was safe to do).

             sage: a,b = var('a b', domain='real')
             sage: A = abs((a+I*b))^2
-            sage: A.canonicalize_radical()
-            a^2 + b^2

comment:17 Changed 5 years ago by jdemeyer

Like I already said, this should be long:

int64_t gethash()

comment:18 Changed 5 years ago by jdemeyer

That's it on first sight.

By the way, I think that this should be pynac-0.6.0 instead of pynax-0.5.4.

comment:19 in reply to: ↑ 15 ; follow-ups: Changed 5 years ago by rws

Replying to jdemeyer:

I don't really understand the doctest changes of the form

-        sage: QQbar(e^(pi*I/3))
+        sage: QQbar(exp(pi*I/3, hold=True))

These I did mindlessly to preserve the result:

Failed example:
    QQbar(e^(pi*I/3))
Expected:
    0.500000000000000? + 0.866025403784439?*I
Got:
    0.50000000000000000? + 0.866025403784439?*I

This one cannot be fixed otherwise:

Failed example:
    a.composition(exp(I*pi/3), exp)
Exception raised:
      File "/home/ralf/sage/local/lib/python2.7/site-packages/sage/symbolic/expression_conversions.py", line 866, in composition
        operand, = ex.operands()
    ValueError: too many values to unpack

Replying to jdemeyer:

Nor this one:

-        sage: N(integrate(sin(x^2)/(x^2), x, 1, infinity))
-        0.285736646322853
+        sage: N(integrate(sin(x^2)/(x^2), x, 1, infinity), prec=40)
+        0.28573664632

How do you let the original succeed? Attempts with tolerance didn't work:

sage: N(integrate(sin(x^2)/(x^2), x, 1, infinity))
0.285736646322853 + 6.93889390390723e-18*I

By the way, I think that this should be pynac-0.6.0 instead of pynax-0.5.4.

Because of the interface change or the amount of changes?

comment:20 in reply to: ↑ 19 Changed 5 years ago by jdemeyer

Replying to rws:

These I did mindlessly to preserve the result:

I think it's better to keep the test and change the result. That would be a more effective test.

By the way, I think that this should be pynac-0.6.0 instead of pynax-0.5.4.

Because of the interface change or the amount of changes?

Because of backward-incompatible changes.

comment:21 in reply to: ↑ 19 ; follow-up: Changed 5 years ago by jdemeyer

Replying to rws:

sage: N(integrate(sin(x^2)/(x^2), x, 1, infinity))
0.285736646322853 + 6.93889390390723e-18*I

That's clearly a bug. You should never change a test in order to hide a bug. If you cannot make it work, use # known bug for this.

comment:22 Changed 5 years ago by rws

  • Authors set to Ralf Stephan
  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from Update to pynac-0.5.4 to Update to pynac-0.6.0

comment:23 Changed 5 years ago by rws

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

comment:24 in reply to: ↑ 21 Changed 5 years ago by jdemeyer

  • Commit changed from ea6a8fac59921b69c8e9caede213a697064e5494 to 2b42c7642ad846b26a645e211e01a0f59e493a7f

Replying to jdemeyer:

That's clearly a bug. You should never change a test in order to hide a bug. If you cannot make it work, use # known bug for this.

Tiny detail: you should keep the correct output (without imag part).


New commits:

2b42c76pynac-0.6.0 pkg version/chksum, symbolic interface, cot/acot, doctest changes

comment:25 Changed 5 years ago by git

  • Commit changed from 2b42c7642ad846b26a645e211e01a0f59e493a7f to 4bd89e6e3be2a94223762a62374c87e0570403b2

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

4bd89e6restore original doctest

comment:26 Changed 5 years ago by jdemeyer

  • Branch changed from u/rws/19819-2 to u/jdemeyer/19819-2

comment:27 Changed 5 years ago by jdemeyer

  • Commit changed from 4bd89e6e3be2a94223762a62374c87e0570403b2 to 26af24d62b9c8ba4cdf1ce7f6406887f1c000262
  • Reviewers set to Jeroen Demeyer

New commits:

26af24dMinor pynac fixes

comment:28 Changed 5 years ago by jdemeyer

I am currently testing this on 32-bit. If that passes and you are happy with my last commit, then this is good to go.

comment:29 Changed 5 years ago by rws

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Ralf Stephan

The changes are fine.

comment:30 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

On 32-bit:

sage -t --long --warn-long 133.0 src/sage/functions/other.py
**********************************************************************
File "src/sage/functions/other.py", line 1684, in sage.functions.other.Function_beta.__init__
Failed example:
    beta(x/2,3)
Expected:
    beta(3, 1/2*x)
Got:
    beta(1/2*x, 3)
**********************************************************************

and

sage -t --long --warn-long 133.0 src/sage/symbolic/expression.pyx
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 11113, in sage.symbolic.expression.Expression._plot_fast_callable
Failed example:
    b
Expected:
    10201.0
Got:
    10200.999999999998
**********************************************************************

comment:31 Changed 5 years ago by git

  • Commit changed from 26af24d62b9c8ba4cdf1ce7f6406887f1c000262 to ae48eff1a88f8b7118674e662b625944f6b3d518

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

ae48effDoctest fixes for 32-bit

comment:32 Changed 5 years ago by jdemeyer

  • Status changed from needs_work to needs_review

For me, this is good to go now.

comment:33 Changed 5 years ago by rws

  • Status changed from needs_review to positive_review

comment:34 Changed 5 years ago by rws

Thanks!

comment:35 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

on OSX:

sage -t --long src/sage/functions/trig.py
**********************************************************************
File "src/sage/functions/trig.py", line 618, in sage.functions.trig.Function_arccot.__init__
Failed example:
    RDF(arccot(1/2))
Expected:
    1.1071487177940904
Got:
    1.1071487177940906
**********************************************************************
1 item had failures:
   1 of  11 in sage.functions.trig.Function_arccot.__init__
    [209 tests, 1 failure, 2.88 s]

comment:36 Changed 5 years ago by rws

The actual value is 1.10714871779409050301 so on Linux it isn't right either. I'm not a FP expert, I guess that changing to ...905 and allowing some tolerance should suffice.

comment:37 Changed 5 years ago by rws

  • Branch changed from u/jdemeyer/19819-2 to u/rws/19819-2

comment:38 Changed 5 years ago by rws

  • Commit changed from ae48eff1a88f8b7118674e662b625944f6b3d518 to e02457e7799d5a5518d27ab64c4809917969843b
  • Status changed from needs_work to needs_review

New commits:

e02457eallow tol because of OSX difference

comment:39 Changed 5 years ago by rws

Doesn't seem to work.

comment:40 Changed 5 years ago by rws

  • Status changed from needs_review to needs_info

I have no idea why changing abs/rel, 1e-14/2e-16, or adding tabs won't work.

comment:41 Changed 5 years ago by rws

  • Branch changed from u/rws/19819-2 to u/jdemeyer/19819-2
  • Commit changed from e02457e7799d5a5518d27ab64c4809917969843b to ae48eff1a88f8b7118674e662b625944f6b3d518

I also cannot pull Jeroen's patches into my branch with git trac pull u/jdemeyer/19819-2 so I set the branch back to his.

EDIT: ok the pull works now


New commits:

26af24dMinor pynac fixes
ae48effDoctest fixes for 32-bit
Last edited 5 years ago by rws (previous) (diff)

comment:42 Changed 5 years ago by jdemeyer

I'll look into the tolerance.

comment:43 Changed 5 years ago by git

  • Commit changed from ae48eff1a88f8b7118674e662b625944f6b3d518 to 5ea3457f5d8459610cc5e0d1547a32c0682e6179

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

5ea3457allow tol because of OSX difference

comment:44 Changed 5 years ago by jdemeyer

  • Status changed from needs_info to positive_review

The # abs tol should be on the doctest line, not on the result line.

comment:45 Changed 5 years ago by rws

Thanks. One could argue the doctest consists of both lines but...

comment:46 Changed 5 years ago by vbraun

I also found this with the previous version some machine, its unlikely to be fixed in the latest commit but I'm running it right now anyways:

sage -t --long src/sage/functions/other.py
**********************************************************************
File "src/sage/functions/other.py", line 1684, in sage.functions.other.Function_beta.__init__
Failed example:
    beta(x/2,3)
Expected:
    beta(1/2*x, 3)  
Got:
    beta(3, 1/2*x)
**********************************************************************
File "src/sage/functions/other.py", line 1691, in sage.functions.other.Function_beta.__init__
Failed example:
    beta(3,x+I)
Expected:
    beta(3, x + I)
Got:
    beta(x + I, 3)
**********************************************************************
1 item had failures:
   2 of  16 in sage.functions.other.Function_beta.__init__
    [491 tests, 2 failures, 16.51 s]

comment:47 follow-up: Changed 5 years ago by rws

While it would certainly be interesting but difficult to find out why this happens, it might also be an idea to write the doctests in a way that ignores order, like using sets.

comment:48 in reply to: ↑ 47 Changed 5 years ago by jdemeyer

Replying to rws:

While it would certainly be interesting but difficult to find out why this happens

I think it has to do with hashes. Something like beta(a, b) gets reordered such that hash(a) < hash(b).

comment:49 Changed 5 years ago by vbraun

I guess ginac orders the beta arguments, which probably makes numerical sense but not with symbolic arguments. Really it should only sort arguments if they are numeric, though thats a minor bug. I'm also fine with just fixing up the doctests.

comment:50 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:51 Changed 5 years ago by jdemeyer

Log files please...

comment:54 Changed 5 years ago by rws

  • Branch changed from u/jdemeyer/19819-2 to u/rws/19819-3

comment:55 Changed 5 years ago by rws

  • Commit changed from 5ea3457f5d8459610cc5e0d1547a32c0682e6179 to 33a4faab1ff70d2d3b2f0d3f61248a61bae57149
  • Status changed from needs_work to needs_review

This would be the order-independence solution.


New commits:

33a4faa19819: make beta function doctests order-independent

comment:56 Changed 5 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:57 Changed 5 years ago by jdemeyer

I just looked into this a bit more and the re-ordering of beta(a,b) indeed depends on the hashes of a and b. Unfortunately, the hashes of expressions are random. So, in the case of

beta(1/2*x, 3)

we know that hash(3) = 3 but the value of hash(1/2*x) is just random.

comment:58 Changed 5 years ago by vbraun

  • Branch changed from u/rws/19819-3 to 33a4faab1ff70d2d3b2f0d3f61248a61bae57149
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.