#21369 closed defect (fixed)
Update to pynac0.6.9
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  packages: standard  Keywords:  
Cc:  embray, tscrim, fbissey  Merged in:  
Authors:  Ralf Stephan, Erik Bray, Jeroen Demeyer  Reviewers:  Jeroen Demeyer, Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  b06cfa0 (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description
Changelog:
 fix bug affecting Cygwin compile (#21265)
 combine some numerics in nested powers; (#21360)
 fix and clean up
ex::coefficients()
(#20455) expand()
denominators too (#21302) make
ex::combine_fractions()
recursive (#20858)  more
normal()
options (#21335)  Appell F1 function skeleton
 updates from GiNaC
https://github.com/pynac/pynac/releases/download/pynac0.6.9/pynac0.6.9.tar.bz2
Change History (28)
comment:1 Changed 3 years ago by
 Branch set to u/rws/update_to_pynac_0_6_9
comment:2 Changed 3 years ago by
 Commit set to a4f58d734817e84d4ba5ba16cbd09587fa635f26
 Status changed from new to needs_review
comment:3 Changed 3 years ago by
This is a regression (based on a doctest you removed):
Before:
sage: k = GF(7); expand((k(1)*x^2 + k(1)*x)^7) x^14 + x^7
After:
sage: k = GF(7); expand((k(1)*x^2 + k(1)*x)^7) x^14 + 0*x^13 + 0*x^12 + 0*x^11 + 0*x^10 + 0*x^9 + 0*x^8 + x^7
comment:4 Changed 3 years ago by
 Status changed from needs_review to needs_info
In general, for any doctest that you remove, it should be clearly justified why that doctest can be removed.
comment:5 followup: ↓ 9 Changed 3 years ago by
Oh well, an impasse. My justification to me personally is that I do not want to fix a feature of symbolics that is better served by algebraic structures in Sage. As it is buggy already now, and we see no complaints, I also suspect that symbolic Mod
is used by a negligible number of people.
I offer to start a sagedevel thread, and to work on a deprecation ticket in case others agree.
comment:6 Changed 3 years ago by
 Commit changed from a4f58d734817e84d4ba5ba16cbd09587fa635f26 to ac7d97159746aec5c68f25f1e628cd18c67ca609
Branch pushed to git repo; I updated commit sha1. New commits:
ac7d971  revert removal of pos.char. doctests; add "known bug"

comment:7 followup: ↓ 8 Changed 3 years ago by
 Cc tscrim added
For # known bug
, you should put in the correct answer.
comment:8 in reply to: ↑ 7 ; followup: ↓ 10 Changed 3 years ago by
Replying to tscrim:
For
# known bug
, you should put in the correct answer.
That is the case here if I'm not mistaken.
comment:9 in reply to: ↑ 5 Changed 3 years ago by
comment:10 in reply to: ↑ 8 Changed 3 years ago by
comment:11 Changed 3 years ago by
comment:12 Changed 3 years ago by
 Status changed from needs_info to needs_review
comment:13 Changed 3 years ago by
Thanks!
comment:14 followup: ↓ 16 Changed 3 years ago by
 The changes to
simplify_rational()
seem more than just a Pynac update. For easier review, I suggest to move those changes to #21335 and focus on the upgrade.
 Why this?
@@ 1387,10 +1387,19 @@ cdef class Expression(CommutativeRingElement): ... TypeError: unable to simplify to float approximation """ + from sage.functions.other import real, imag try:  return float(self._eval_self(float)) + ret = float(self._eval_self(float)) except TypeError:  raise TypeError("unable to simplify to float approximation") + try: + c = (self._eval_self(complex)) + if imag(c) == 0: + ret = real(c) + else: + raise + except TypeError: + raise TypeError("unable to simplify to float approximation") + return ret def __complex__(self):
I understand and see that it makes sense, I am just wondering which bug it fixes.
 The code mentioned in 2. could be improved a bit. Instead of using the
ret
variable, I suggest just returning the value directly. In the complex case, I would usec = complex(self._eval_self(complex))
and then use thereal
andimag
attributes (not functions).
 Can you justify

src/sage/symbolic/expression.pyx
diff git a/src/sage/symbolic/expression.pyx b/src/sage/symbolic/expression.pyx index 6c4171f..d780bbc 100644
a b cdef class Expression(CommutativeRingElement): 1191 1191 except TypeError as err: 1192 1192 # try the evaluation again with the complex field 1193 1193 # corresponding to the parent R 1194 if R i s float:1194 if R in (float, complex): 1195 1195 R_complex = complex 1196 1196 else: 1197 1197 try:

It seems pointless to have R_complex == R
.
comment:15 Changed 3 years ago by
 Commit changed from ac7d97159746aec5c68f25f1e628cd18c67ca609 to 3dd8058f01117093516a52851e43ffb645b30c89
Branch pushed to git repo; I updated commit sha1. New commits:
3dd8058  address reviewer's comments

comment:16 in reply to: ↑ 14 ; followups: ↓ 18 ↓ 19 Changed 3 years ago by
Replying to jdemeyer:
 The changes to
simplify_rational()
seem more than just a Pynac update. For easier review, I suggest to move those changes to #21335 and focus on the upgrade.
OK.
 Why this?
...
I understand and see that it makes sense, I am just wondering which bug it fixes.
sage: float(sqrt(2)/sqrt(abs((I  1)*sqrt(2)  I  1)))  TypeError Traceback (most recent call last) <ipythoninput95bba6e8d1a16> in <module>() > 1 float(sqrt(Integer(2))/sqrt(abs((I  Integer(1))*sqrt(Integer(2))  I  Integer(1)))) /home/ralf/sage/src/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression.__float__ (build/cythonized/sage/symbolic/expression.cpp:10473)() 1391 return float(self._eval_self(float)) 1392 except TypeError: > 1393 raise TypeError("unable to simplify to float approximation") 1394 1395 def __complex__(self): TypeError: unable to simplify to float approximation
I was unable to find out why this bombed but it does so at least since Sage7.1.
 Can you justify
+ if R in (float, complex):It seems pointless to have
R_complex == R
.
Yes but this also prevents the else
branch done (R.complex_field()
with R
being complex
).
New commits:
3dd8058  address reviewer's comments

New commits:
3dd8058  address reviewer's comments

comment:17 Changed 3 years ago by
 Commit changed from 3dd8058f01117093516a52851e43ffb645b30c89 to 1f29305b6ca0e176910ceeb18156081d3dface11
Branch pushed to git repo; I updated commit sha1. New commits:
1f29305  add doctest

comment:18 in reply to: ↑ 16 Changed 3 years ago by
Replying to rws:
sage: float(sqrt(2)/sqrt(abs((I  1)*sqrt(2)  I  1)))  TypeError Traceback (most recent call last) <ipythoninput95bba6e8d1a16> in <module>() > 1 float(sqrt(Integer(2))/sqrt(abs((I  Integer(1))*sqrt(Integer(2))  I  Integer(1)))) /home/ralf/sage/src/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression.__float__ (build/cythonized/sage/symbolic/expression.cpp:10473)() 1391 return float(self._eval_self(float)) 1392 except TypeError: > 1393 raise TypeError("unable to simplify to float approximation") 1394 1395 def __complex__(self): TypeError: unable to simplify to float approximationI was unable to find out why this bombed but it does so at least since Sage7.1.
Can you add this as doctest then?
And delete the line from sage.functions.other import real, imag
comment:19 in reply to: ↑ 16 Changed 3 years ago by
Replying to rws:
Yes but this also prevents the
else
branch done (R.complex_field()
withR
beingcomplex
).
And why is that a problem? Sure, it will raise an AttributeError
, but that's caught. So I still don't see the point.
comment:20 Changed 3 years ago by
I am happy with the branch on this ticket except for the float
/complex
stuff in expression.pyx
that I need to look at more.
comment:21 Changed 3 years ago by
 Commit changed from 1f29305b6ca0e176910ceeb18156081d3dface11 to 0065f62b563eb6a7e67bd61fdba8d888a4d7e3f8
Branch pushed to git repo; I updated commit sha1. New commits:
0065f62  address reviewer's comments

comment:22 Changed 3 years ago by
 Branch changed from u/rws/update_to_pynac_0_6_9 to u/jdemeyer/update_to_pynac_0_6_9
comment:23 Changed 3 years ago by
 Commit changed from 0065f62b563eb6a7e67bd61fdba8d888a4d7e3f8 to b06cfa068ae53d9bcb016a0bc4aba1ba21c8bc72
 Reviewers set to Jeroen Demeyer
I fixed the underlying cause of the float
/complex
problem which is in _eval_self
. This way, no changes to __float__
are needed. If you agree with this, you can set the ticket to positive_review.
New commits:
b06cfa0  Fix _eval_self(float) for "real" complex expressions

comment:24 Changed 3 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Ralf Stephan
 Status changed from needs_review to positive_review
Is fine and passes tests. Thanks.
comment:25 Changed 3 years ago by
This also built without problems on Cygwin32/64 for me.
comment:26 Changed 3 years ago by
 Cc fbissey added
comment:27 Changed 3 years ago by
 Branch changed from u/jdemeyer/update_to_pynac_0_6_9 to b06cfa068ae53d9bcb016a0bc4aba1ba21c8bc72
 Resolution set to fixed
 Status changed from positive_review to closed
comment:28 Changed 3 years ago by
 Commit b06cfa068ae53d9bcb016a0bc4aba1ba21c8bc72 deleted
New commits:
version/chksum
changes affecting Sage behaviour or interface
doctest fixes