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: sage-8.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:

Status badges

Description

sage: R.<r>=SR[]
sage: R
Univariate Polynomial Ring in r over Symbolic Ring
sage: p=r^2-1
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)
<ipython-input-44-4c09f14dea4d> in <module>()
----> 1 p(Integer(1))

/home/dima/Sage/sage-dev/local/lib/python2.7/site-packages/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/sage-dev/local/lib/python2.7/site-packages/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/sage-dev/local/lib/python2.7/site-packages/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 sage-support here

Change History (23)

comment:1 Changed 5 years ago by Ralf Stephan

p.__call__ calls SR(1)._evaluate_polynomial:

        /* "sage/rings/polynomial/polynomial_element.pyx":790
 *         elif hasattr(a, "_evaluate_polynomial"):
 *             try:
 *                 return a._evaluate_polynomial(pol)             # <<<<<<<<<<<<<<
 *             except NotImplementedError:
 *                 pass
 */

and SR(1)._evaluate_polynomial calls p(SR(1)):

  /* "sage/symbolic/expression.pyx":7459
 *         """
 *         cdef Expression zero
 *         try:             # <<<<<<<<<<<<<<
 *             return new_Expression_from_pyobject(self._parent, pol(self.pyobject()))
 *         except TypeError:
 */

comment:2 Changed 5 years ago by Ralf Stephan

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 Ralf Stephan

Component: symbolicscommutative algebra

comment:4 Changed 5 years ago by Ralf Stephan

Cc: Marc Mezzarobba added

Maybe instead of pol(self.pyobject()) parent(pol)(self.pyobject()) was meant?

comment:5 Changed 5 years ago by Ralf Stephan

Branch: u/rws/substitution_into_polynomials_over_sr_broken

comment:6 Changed 5 years ago by git

Commit: 6e298a11d546988258d1842860d2ef577a3b7baa

Branch pushed to git repo; I updated commit sha1. New commits:

6e298a124853: fix doctests

comment:7 Changed 5 years ago by Ralf Stephan

Authors: Ralf Stephan
Status: newneeds_review

This also fixes doctests that were wrong IMHO.

Finally, I still see a half-second delay when doing the first R.<r>=SR[]; (r^2-1)(1).

comment:8 in reply to:  7 Changed 5 years ago by Dima Pasechnik

Replying to rws:

Finally, I still see a half-second delay when doing the first R.<r>=SR[]; (r^2-1)(1).

Something big is getting loaded, perhaps Maxima?

comment:9 Changed 5 years ago by Jeroen Demeyer

Polynomials over SR really make no sense in the first place.

comment:10 in reply to:  9 Changed 5 years ago by Ralf Stephan

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 ; Changed 5 years ago by Marc Mezzarobba

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 5 years ago by Marc Mezzarobba

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 ; Changed 5 years ago by Ralf Stephan

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 5 years ago by Ralf Stephan

Replying to mmezzarobba:

make more sense?

Please go ahead.

comment:15 in reply to:  13 Changed 5 years ago by Dima Pasechnik

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/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.

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 ; Changed 5 years ago by Marc Mezzarobba

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/5000

Because pol is a QQ[] and QQ(0.1) is 1/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 5 years ago by Vincent Delecroix

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/5000

Because pol is a QQ[] and QQ(0.1) is 1/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).

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 Ralf Stephan

Authors: Ralf Stephan
Branch: u/rws/substitution_into_polynomials_over_sr_broken
Commit: 6e298a11d546988258d1842860d2ef577a3b7baa
Status: needs_reviewneeds_work

I think who introduced the bug should also fix it.

comment:19 Changed 5 years ago by Marc Mezzarobba

Authors: Marc Mezzarobba
Branch: u/mmezzarobba/ticket/24853-pol-SR
Commit: 598e9bc83f8e2869c9af1fefbdef9b41a8254f12
Status: needs_workneeds_review

I have no idea who introduced the bug, but here is a possible fix that makes sense to me.

Last edited 5 years ago by Marc Mezzarobba (previous) (diff)

comment:20 Changed 5 years ago by git

Commit: 598e9bc83f8e2869c9af1fefbdef9b41a8254f128301d6b7fddc49b190363a42c4828ae8dccc4b7c

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 git

Commit: 8301d6b7fddc49b190363a42c4828ae8dccc4b7cdbe38b1b2a303cac558907269d995c80c8be1088

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 Ralf Stephan

Reviewers: Ralf Stephan
Status: needs_reviewpositive_review

Thanks. It's looking fine and patchbot has only the test_jupyter fail.

comment:23 Changed 5 years ago by Volker Braun

Branch: u/mmezzarobba/ticket/24853-pol-SRdbe38b1b2a303cac558907269d995c80c8be1088
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.