Opened 5 years ago
Closed 5 years ago
#19819 closed enhancement (fixed)
Update to pynac0.6.0
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage7.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: 
Description (last modified by )
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 forPi
andI
before trying to divide  performance: much less canonicalizations of GMP rationals
 performance: cache pseries coeff accesses in
pseries::mul_series
 performance:
GinacFunction
s forcot/acot
 C++11: add
override
where possible
https://github.com/pynac/pynac/releases/download/pynac0.6.0/pynac0.6.0.tar.bz2
Change History (58)
comment:1 in reply to: ↑ description ; followup: ↓ 2 Changed 5 years ago by
comment:2 in reply to: ↑ 1 ; followup: ↓ 3 Changed 5 years ago by
Replying to jdemeyer:
Replying to rws:
 change hash type to
int64_t
(#19310)Why
int64_t
? The type should belong
.
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 ; followup: ↓ 4 Changed 5 years ago by
comment:4 in reply to: ↑ 3 ; followup: ↓ 7 Changed 5 years ago by
comment:5 followup: ↓ 6 Changed 5 years ago by
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
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 searchandreplace int64_t
> long
would probably work.
comment:7 in reply to: ↑ 4 Changed 5 years ago by
comment:8 followups: ↓ 9 ↓ 10 Changed 5 years ago by
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
comment:10 in reply to: ↑ 8 Changed 5 years ago by
Replying to rws:
sage: hash(SR(2^32)) 0This 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
 Branch set to u/rws/198191
comment:12 Changed 5 years ago by
 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:
ea6a8fa  proposed changes because of pynac0.5.4

comment:13 Changed 5 years ago by
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 32bit.
comment:14 Changed 5 years ago by
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 followup: ↓ 19 Changed 5 years ago by
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
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
Like I already said, this should be long
:
int64_t gethash()
comment:18 Changed 5 years ago by
That's it on first sight.
By the way, I think that this should be pynac0.6.0 instead of pynax0.5.4.
comment:19 in reply to: ↑ 15 ; followups: ↓ 20 ↓ 21 Changed 5 years ago by
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/sitepackages/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.93889390390723e18*I
By the way, I think that this should be pynac0.6.0 instead of pynax0.5.4.
Because of the interface change or the amount of changes?
comment:20 in reply to: ↑ 19 Changed 5 years ago by
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 pynac0.6.0 instead of pynax0.5.4.
Because of the interface change or the amount of changes?
Because of backwardincompatible changes.
comment:21 in reply to: ↑ 19 ; followup: ↓ 24 Changed 5 years ago by
Replying to rws:
sage: N(integrate(sin(x^2)/(x^2), x, 1, infinity)) 0.285736646322853 + 6.93889390390723e18*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
 Description modified (diff)
 Status changed from new to needs_review
 Summary changed from Update to pynac0.5.4 to Update to pynac0.6.0
comment:23 Changed 5 years ago by
 Branch changed from u/rws/198191 to u/rws/198192
comment:24 in reply to: ↑ 21 Changed 5 years ago by
 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:
2b42c76  pynac0.6.0 pkg version/chksum, symbolic interface, cot/acot, doctest changes

comment:25 Changed 5 years ago by
 Commit changed from 2b42c7642ad846b26a645e211e01a0f59e493a7f to 4bd89e6e3be2a94223762a62374c87e0570403b2
Branch pushed to git repo; I updated commit sha1. New commits:
4bd89e6  restore original doctest

comment:26 Changed 5 years ago by
 Branch changed from u/rws/198192 to u/jdemeyer/198192
comment:27 Changed 5 years ago by
 Commit changed from 4bd89e6e3be2a94223762a62374c87e0570403b2 to 26af24d62b9c8ba4cdf1ce7f6406887f1c000262
 Reviewers set to Jeroen Demeyer
New commits:
26af24d  Minor pynac fixes

comment:28 Changed 5 years ago by
I am currently testing this on 32bit. If that passes and you are happy with my last commit, then this is good to go.
comment:29 Changed 5 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Ralf Stephan
The changes are fine.
comment:30 Changed 5 years ago by
 Status changed from needs_review to needs_work
On 32bit:
sage t long warnlong 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 warnlong 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
 Commit changed from 26af24d62b9c8ba4cdf1ce7f6406887f1c000262 to ae48eff1a88f8b7118674e662b625944f6b3d518
Branch pushed to git repo; I updated commit sha1. New commits:
ae48eff  Doctest fixes for 32bit

comment:32 Changed 5 years ago by
 Status changed from needs_work to needs_review
For me, this is good to go now.
comment:33 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:34 Changed 5 years ago by
Thanks!
comment:35 Changed 5 years ago by
 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
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
 Branch changed from u/jdemeyer/198192 to u/rws/198192
comment:38 Changed 5 years ago by
 Commit changed from ae48eff1a88f8b7118674e662b625944f6b3d518 to e02457e7799d5a5518d27ab64c4809917969843b
 Status changed from needs_work to needs_review
New commits:
e02457e  allow tol because of OSX difference

comment:39 Changed 5 years ago by
Doesn't seem to work.
comment:40 Changed 5 years ago by
 Status changed from needs_review to needs_info
I have no idea why changing abs/rel
, 1e14/2e16
, or adding tabs won't work.
comment:41 Changed 5 years ago by
 Branch changed from u/rws/198192 to u/jdemeyer/198192
 Commit changed from e02457e7799d5a5518d27ab64c4809917969843b to ae48eff1a88f8b7118674e662b625944f6b3d518
comment:42 Changed 5 years ago by
I'll look into the tolerance.
comment:43 Changed 5 years ago by
 Commit changed from ae48eff1a88f8b7118674e662b625944f6b3d518 to 5ea3457f5d8459610cc5e0d1547a32c0682e6179
Branch pushed to git repo; I updated commit sha1. New commits:
5ea3457  allow tol because of OSX difference

comment:44 Changed 5 years ago by
 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
Thanks. One could argue the doctest consists of both lines but...
comment:46 Changed 5 years ago by
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 followup: ↓ 48 Changed 5 years ago by
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
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
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
 Status changed from positive_review to needs_work
comment:51 Changed 5 years ago by
Log files please...
comment:52 Changed 5 years ago by
comment:53 Changed 5 years ago by
comment:54 Changed 5 years ago by
 Branch changed from u/jdemeyer/198192 to u/rws/198193
comment:55 Changed 5 years ago by
 Commit changed from 5ea3457f5d8459610cc5e0d1547a32c0682e6179 to 33a4faab1ff70d2d3b2f0d3f61248a61bae57149
 Status changed from needs_work to needs_review
This would be the orderindependence solution.
New commits:
33a4faa  19819: make beta function doctests orderindependent

comment:56 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:57 Changed 5 years ago by
I just looked into this a bit more and the reordering 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
 Branch changed from u/rws/198193 to 33a4faab1ff70d2d3b2f0d3f61248a61bae57149
 Resolution set to fixed
 Status changed from positive_review to closed
Replying to rws:
Why
int64_t
? The type should belong
.