#24853 closed defect (fixed)

substitution into polynomials over SR broken

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.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^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 16 months ago by rws

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 16 months ago by 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__()

comment:3 Changed 16 months ago by rws

  • Component changed from symbolics to commutative algebra

comment:4 Changed 16 months ago by rws

  • Cc mmezzarobba added

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

comment:5 Changed 16 months ago by rws

  • Branch set to u/rws/substitution_into_polynomials_over_sr_broken

comment:6 Changed 16 months ago by git

  • Commit set to 6e298a11d546988258d1842860d2ef577a3b7baa

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

6e298a124853: fix doctests

comment:7 follow-ups: Changed 16 months ago by rws

  • Authors set to Ralf Stephan
  • Status changed from new to needs_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 16 months ago by dimpase

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 follow-ups: Changed 16 months ago by jdemeyer

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

comment:10 in reply to: ↑ 9 Changed 16 months ago by rws

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 ; follow-ups: Changed 16 months ago by mmezzarobba

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 16 months ago by 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.

comment:13 in reply to: ↑ 11 ; follow-ups: Changed 16 months ago by 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.

comment:14 in reply to: ↑ 11 Changed 16 months ago by rws

Replying to mmezzarobba:

make more sense?

Please go ahead.

comment:15 in reply to: ↑ 13 Changed 16 months ago by dimpase

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 ; follow-up: Changed 16 months ago by 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).

comment:17 in reply to: ↑ 16 Changed 15 months ago by vdelecroix

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 15 months ago by rws

  • Authors Ralf Stephan deleted
  • 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 15 months ago by mmezzarobba

  • Authors set to Marc Mezzarobba
  • Branch set to u/mmezzarobba/ticket/24853-pol-SR
  • 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.

Last edited 15 months ago by mmezzarobba (previous) (diff)

comment:20 Changed 15 months ago by git

  • 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 15 months ago by git

  • 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 15 months ago by rws

  • 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 15 months ago by vbraun

  • Branch changed from u/mmezzarobba/ticket/24853-pol-SR to dbe38b1b2a303cac558907269d995c80c8be1088
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.