Opened 20 months ago
Closed 19 months ago
#24853 closed defect (fixed)
substitution into polynomials over SR broken
Reported by:  dimpase  Owned by:  

Priority:  major  Milestone:  sage8.2 
Component:  commutative algebra  Keywords:  
Cc:  rws, mmezzarobba  Merged in:  
Authors:  Marc Mezzarobba  Reviewers:  Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  dbe38b1 (Commits)  Commit:  dbe38b1b2a303cac558907269d995c80c8be1088 
Dependencies:  Stopgaps: 
Description
sage: R.<r>=SR[] sage: R Univariate Polynomial Ring in r over Symbolic Ring sage: p=r^21 sage: p(1) Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <type 'exceptions.RuntimeError'> ignored  RuntimeError Traceback (most recent call last) <ipythoninput444c09f14dea4d> in <module>() > 1 p(Integer(1)) /home/dima/Sage/sagedev/local/lib/python2.7/sitepackages/sage/rings/polynomial/polynomial_element.pyx in sage.rings.polynomial.polynomial_element.Polynomial.__call__ (build/cythonized/sage/rings/polynomial/polynomial_element.c:9535)() 787 elif hasattr(a, "_evaluate_polynomial"): 788 try: > 789 return a._evaluate_polynomial(pol) 790 except NotImplementedError: 791 pass /home/dima/Sage/sagedev/local/lib/python2.7/sitepackages/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression._evaluate_polynomial (build/cythonized/sage/symbolic/expression.cpp:45786)() 7458 cdef Expression zero 7459 try: > 7460 return new_Expression_from_pyobject(self._parent, pol(self.pyobject())) 7461 except TypeError: 7462 zero = self._parent.zero() ... last 2 frames repeated, from the frame below ... /home/dima/Sage/sagedev/local/lib/python2.7/sitepackages/sage/rings/polynomial/polynomial_element.pyx in sage.rings.polynomial.polynomial_element.Polynomial.__call__ (build/cythonized/sage/rings/polynomial/polynomial_element.c:9535)() 787 elif hasattr(a, "_evaluate_polynomial"): 788 try: > 789 return a._evaluate_polynomial(pol) 790 except NotImplementedError: 791 pass RuntimeError: maximum recursion depth exceeded while calling a Python object
see also sagesupport here
Change History (23)
comment:1 Changed 20 months ago by
comment:2 Changed 20 months ago by
The changes came here:
commit 03e86910b66e18bc652b918a1ad8e2892ac04004 Author: Marc Mezzarobba <marc@mezzarobba.net> Date: Wed Jan 11 12:18:50 2017 +0100 speed up generic Polynomial.__call__()
comment:3 Changed 20 months ago by
 Component changed from symbolics to commutative algebra
comment:4 Changed 20 months ago by
 Cc mmezzarobba added
Maybe instead of pol(self.pyobject())
parent(pol)(self.pyobject())
was meant?
comment:5 Changed 20 months ago by
 Branch set to u/rws/substitution_into_polynomials_over_sr_broken
comment:6 Changed 20 months ago by
 Commit set to 6e298a11d546988258d1842860d2ef577a3b7baa
Branch pushed to git repo; I updated commit sha1. New commits:
6e298a1  24853: fix doctests

comment:7 followups: ↓ 8 ↓ 11 Changed 20 months ago by
 Status changed from new to needs_review
This also fixes doctests that were wrong IMHO.
Finally, I still see a halfsecond delay when doing the first R.<r>=SR[]; (r^21)(1)
.
comment:8 in reply to: ↑ 7 Changed 20 months ago by
Replying to rws:
Finally, I still see a halfsecond delay when doing the first
R.<r>=SR[]; (r^21)(1)
.
Something big is getting loaded, perhaps Maxima?
comment:9 followups: ↓ 10 ↓ 12 Changed 20 months ago by
Polynomials over SR
really make no sense in the first place.
comment:10 in reply to: ↑ 9 Changed 20 months ago by
Replying to jdemeyer:
Polynomials over
SR
really make no sense in the first place.
charpoly
of a symbolic matrix should return a symbolic polynomial?
comment:11 in reply to: ↑ 7 ; followups: ↓ 13 ↓ 14 Changed 20 months ago by
Replying to rws:
The changes came here:
commit 03e86910b66e18bc652b918a1ad8e2892ac04004 Author: Marc Mezzarobba <marc@mezzarobba.net> Date: Wed Jan 11 12:18:50 2017 +0100 speed up generic Polynomial.__call__()
Yes and no: this commit (and, perhaps more importantly, 84d6a1b874c) mainly reorganize the code, without changing its semantics much...
Replying to rws:
Maybe instead of
pol(self.pyobject())
parent(pol)(self.pyobject())
was meant?
...But I doubt that. Wouldn't something like (not tested)
if pol.parent() is not SR: try: return new_Expression_from_pyobject(self._parent, pol(self.pyobject())) except TypeError: pass zero = self._parent.zero() return zero.add(*(pol[i]*self**i for i in xrange(pol.degree() + 1)))
make more sense?
Replying to rws:
This also fixes doctests that were wrong IMHO.
While I have no strong opinion about
sage: f(pi)  1.00000000000000*pi^2  2.00000000000000  + 7.86960440108936
I really don't see why you would want this:
sage: SR(0.1)._evaluate_polynomial(pol)  0.123400000000000 + 617/5000
comment:12 in reply to: ↑ 9 Changed 20 months ago by
Replying to jdemeyer:
Polynomials over
SR
really make no sense in the first place.
Why? I agree with Ralf here, characteristic polynomials of symbolic matrices are a good example.
comment:13 in reply to: ↑ 11 ; followups: ↓ 15 ↓ 16 Changed 20 months ago by
Replying to mmezzarobba:
I really don't see why you would want this:
sage: SR(0.1)._evaluate_polynomial(pol)  0.123400000000000 + 617/5000
Because pol
is a QQ[]
and QQ(0.1)
is 1/10
of course.
Replying to mmezzarobba:
Replying to jdemeyer:
Polynomials over
SR
really make no sense in the first place.Why? I agree with Ralf here, characteristic polynomials of symbolic matrices are a good example.
Don't misunderstand me, my comment was not a rethorical question. I'm ready to change what's returned from charpoly() to an expression.
comment:14 in reply to: ↑ 11 Changed 20 months ago by
comment:15 in reply to: ↑ 13 Changed 20 months ago by
Replying to rws:
Replying to mmezzarobba:
I really don't see why you would want this:
sage: SR(0.1)._evaluate_polynomial(pol)  0.123400000000000 + 617/5000Because
pol
is aQQ[]
andQQ(0.1)
is1/10
of course.Replying to mmezzarobba:
Replying to jdemeyer:
Polynomials over
SR
really make no sense in the first place.Why? I agree with Ralf here, characteristic polynomials of symbolic matrices are a good example.
Don't misunderstand me, my comment was not a rethorical question. I'm ready to change what's returned from charpoly() to an expression.
I would not try to force this onto the user  this conversion is easy.
I also do not understand why having polynomials over SR
makes no sense; well, it might be so in an ideal world where SR
can seamlessly discover polynomial structures in expressions and operate accordingly, but this is not the case I think.
comment:16 in reply to: ↑ 13 ; followup: ↓ 17 Changed 20 months ago by
Replying to rws:
Replying to mmezzarobba:
I really don't see why you would want this:
sage: SR(0.1)._evaluate_polynomial(pol)  0.123400000000000 + 617/5000Because
pol
is aQQ[]
andQQ(0.1)
is1/10
of course.
I think I don't understand what you mean, sorry. First, QQ(0.1)
is 1/10, but there is no coercion from RR
to QQ
so this doesn't mean anything, and (0.1).exact_rational()
is 3602879701896397/36028797018963968
. Second, even 0.1 “was” 1/10 in some stronger sense, I don't see why polynomial evaluation would want to convert the argument to the polynomial's base ring (outside symbolics, Polynomial.__call__()
returns an element of the pushout of the base ring and the evaluation point, which does the right thing in every(?) case).
comment:17 in reply to: ↑ 16 Changed 20 months ago by
Replying to mmezzarobba:
Replying to rws:
Replying to mmezzarobba:
I really don't see why you would want this:
sage: SR(0.1)._evaluate_polynomial(pol)  0.123400000000000 + 617/5000Because
pol
is aQQ[]
andQQ(0.1)
is1/10
of course.I think I don't understand what you mean, sorry. First,
QQ(0.1)
is 1/10, but there is no coercion fromRR
to(0.1).exact_rational()
is3602879701896397/36028797018963968
. Second, even 0.1 “was” 1/10 in some stronger sense, I don't see why polynomial evaluation would want to convert the argument to the polynomial's base ring (outside symbolics,Polynomial.__call__()
returns an element of the pushout of the base ring and the evaluation point, which does the right thing in every(?) case).
I fully agree with Marc here (see laso the related discussion at #24939). If there is any floating point number in the polynomial, then the output can never be an exact number (including elements of QQ, AA, QQbar as well as exact constants such as pi, e, etc, arctan(2)).
comment:18 Changed 20 months ago by
 Branch u/rws/substitution_into_polynomials_over_sr_broken deleted
 Commit 6e298a11d546988258d1842860d2ef577a3b7baa deleted
 Status changed from needs_review to needs_work
I think who introduced the bug should also fix it.
comment:19 Changed 20 months ago by
 Branch set to u/mmezzarobba/ticket/24853polSR
 Commit set to 598e9bc83f8e2869c9af1fefbdef9b41a8254f12
 Status changed from needs_work to needs_review
I have no idea who introduced the bug, but here is a possible fix that makes sense to me.
comment:20 Changed 20 months ago by
 Commit changed from 598e9bc83f8e2869c9af1fefbdef9b41a8254f12 to 8301d6b7fddc49b190363a42c4828ae8dccc4b7c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8301d6b  #24853 fix infinite recursion in polynomial evaluation

comment:21 Changed 20 months ago by
 Commit changed from 8301d6b7fddc49b190363a42c4828ae8dccc4b7c to dbe38b1b2a303cac558907269d995c80c8be1088
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
dbe38b1  #24853 fix infinite recursion in polynomial evaluation

comment:22 Changed 19 months ago by
 Reviewers set to Ralf Stephan
 Status changed from needs_review to positive_review
Thanks. It's looking fine and patchbot has only the test_jupyter fail.
comment:23 Changed 19 months ago by
 Branch changed from u/mmezzarobba/ticket/24853polSR to dbe38b1b2a303cac558907269d995c80c8be1088
 Resolution set to fixed
 Status changed from positive_review to closed
p.__call__
callsSR(1)._evaluate_polynomial
:and
SR(1)._evaluate_polynomial
callsp(SR(1))
: