Opened 5 years ago
Closed 5 years ago
#24853 closed defect (fixed)
substitution into polynomials over SR broken
Reported by:  Dima Pasechnik  Owned by:  

Priority:  major  Milestone:  sage8.2 
Component:  commutative algebra  Keywords:  
Cc:  Ralf Stephan, Marc Mezzarobba  Merged in:  
Authors:  Marc Mezzarobba  Reviewers:  Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  dbe38b1 (Commits, GitHub, GitLab)  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 5 years ago by
comment:2 Changed 5 years 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 5 years ago by
Component:  symbolics → commutative algebra 

comment:4 Changed 5 years ago by
Cc:  Marc Mezzarobba added 

Maybe instead of pol(self.pyobject())
parent(pol)(self.pyobject())
was meant?
comment:5 Changed 5 years ago by
Branch:  → u/rws/substitution_into_polynomials_over_sr_broken 

comment:6 Changed 5 years ago by
Commit:  → 6e298a11d546988258d1842860d2ef577a3b7baa 

Branch pushed to git repo; I updated commit sha1. New commits:
6e298a1  24853: fix doctests

comment:7 followups: 8 11 Changed 5 years ago by
Authors:  → Ralf Stephan 

Status:  new → 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 Changed 5 years 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 5 years ago by
Polynomials over SR
really make no sense in the first place.
comment:10 Changed 5 years 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 followups: 13 14 Changed 5 years 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 Changed 5 years 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 followups: 15 16 Changed 5 years 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:15 Changed 5 years 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 followup: 17 Changed 5 years 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 Changed 5 years 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 5 years ago by
Authors:  Ralf Stephan 

Branch:  u/rws/substitution_into_polynomials_over_sr_broken 
Commit:  6e298a11d546988258d1842860d2ef577a3b7baa 
Status:  needs_review → needs_work 
I think who introduced the bug should also fix it.
comment:19 Changed 5 years ago by
Authors:  → Marc Mezzarobba 

Branch:  → u/mmezzarobba/ticket/24853polSR 
Commit:  → 598e9bc83f8e2869c9af1fefbdef9b41a8254f12 
Status:  needs_work → needs_review 
I have no idea who introduced the bug, but here is a possible fix that makes sense to me.
comment:20 Changed 5 years ago by
Commit:  598e9bc83f8e2869c9af1fefbdef9b41a8254f12 → 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 5 years ago by
Commit:  8301d6b7fddc49b190363a42c4828ae8dccc4b7c → 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 5 years ago by
Reviewers:  → Ralf Stephan 

Status:  needs_review → positive_review 
Thanks. It's looking fine and patchbot has only the test_jupyter fail.
comment:23 Changed 5 years ago by
Branch:  u/mmezzarobba/ticket/24853polSR → dbe38b1b2a303cac558907269d995c80c8be1088 

Resolution:  → fixed 
Status:  positive_review → closed 
p.__call__
callsSR(1)._evaluate_polynomial
:and
SR(1)._evaluate_polynomial
callsp(SR(1))
: