#21369 closed defect (fixed)
Update to pynac-0.6.9
Reported by: | rws | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.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, GitHub, GitLab) | 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/pynac-0.6.9/pynac-0.6.9.tar.bz2
Change History (28)
comment:1 Changed 6 years ago by
- Branch set to u/rws/update_to_pynac_0_6_9
comment:2 Changed 6 years ago by
- Commit set to a4f58d734817e84d4ba5ba16cbd09587fa635f26
- Status changed from new to needs_review
comment:3 Changed 6 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 6 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 follow-up: ↓ 9 Changed 6 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 sage-devel thread, and to work on a deprecation ticket in case others agree.
comment:6 Changed 6 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 follow-up: ↓ 8 Changed 6 years ago by
- Cc tscrim added
For # known bug
, you should put in the correct answer.
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 6 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 6 years ago by
comment:10 in reply to: ↑ 8 Changed 6 years ago by
comment:11 Changed 6 years ago by
comment:12 Changed 6 years ago by
- Status changed from needs_info to needs_review
comment:13 Changed 6 years ago by
Thanks!
comment:14 follow-up: ↓ 16 Changed 6 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 6 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 ; follow-ups: ↓ 18 ↓ 19 Changed 6 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) <ipython-input-9-5bba6e8d1a16> 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 Sage-7.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 6 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 6 years ago by
Replying to rws:
sage: float(sqrt(2)/sqrt(abs(-(I - 1)*sqrt(2) - I - 1))) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-9-5bba6e8d1a16> 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 Sage-7.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 6 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 6 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 6 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 6 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 6 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 6 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 6 years ago by
This also built without problems on Cygwin32/64 for me.
comment:26 Changed 6 years ago by
- Cc fbissey added
comment:27 Changed 6 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 6 years ago by
- Commit b06cfa068ae53d9bcb016a0bc4aba1ba21c8bc72 deleted
New commits:
version/chksum
changes affecting Sage behaviour or interface
doctest fixes