Opened 6 years ago

Closed 4 years ago

#13711 closed defect (fixed)

charpoly error on matrices with pi

Reported by: eviatarbach Owned by: jason, was
Priority: major Milestone: sage-6.6
Component: linear algebra Keywords:
Cc: Merged in:
Authors: Vincent Delecroix Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 005cd02 (Commits) Commit: 005cd02119165e28b2c7a9ae784e447289cfe859
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

Sage returns an error when attempting to calculate the characteristic polynomial of a matrix with pi:

sage: matrix([[sqrt(2), -1], [1, e^2]]).charpoly()    
x^2 + (-sqrt(2) - e^2)*x + sqrt(2)*e^2 + 1
sage: matrix([[sqrt(2), -1], [pi, e^2]]).charpoly()
TypeError

This is fixed in sage-6.6. The branch just add a doctest to the method charpoly of symbolic matrices.

Attachments (1)

trac_13711.patch (1.3 KB) - added by nbruin 6 years ago.
ensure that SymbolicConstant? object convert into SR

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by eviatarbach

Okay, I think I found the source of the error; Sage tries to coerce the pyobjects of the symbolic constants into the Symbolic Ring, which doesn't work:

sage: SR(pi.pyobject())
TypeError
sage: SR(euler_gamma.pyobject())
TypeError

What would be the best way to fix this? It seems to be quite a major problem, since characteristic polynomials cannot at the moment be computed for any matrices with symbolic constants (it does work for e, since it is defined differently, and golden_ratio, since it is replaced with its numerical value before running into this error).

comment:2 Changed 6 years ago by nbruin

The right way of turning a constant into an expression is apparently via the expression method. The element constructor for SR tests for the presence of a _symbolic_ method on its argument. If we add a method to sage.symbolic.constants.Constant:

    def _symbolic_(self,SR):
        """
        doctest:
            sage: SR(sage.symbolic.constants.Constant('a'))
            a
            sage: SR(sage.symbolic.constants.Constant('a')).subs(a=10)
            a
        """
        return SR(self.expression())

the system picks up the conversion. The double conversion that's happening here is a little sad, but the interface for _symbolic_ is that the ring in question is given as a parameter. I guess our design does not rule out the existence of multiple symbolic rings, whereas expression will return a member of whatever symbolic ring is considered "home" by pynac.

One solution would be to give sage.symbolic.constants_c.PynacConstant.expression an optional argument SR to specify in which ring the expression should lie (defaulting to what SR is there now). This routine now just reads:

    def expression(self):
       return new_Expression_from_GEx(SR, <GEx>(self.pointer[0]))

so making that def expression(self, SR=SR) shouldn't make too much difference. With that in place we can just alias _symbolic_ to expression.

Last edited 6 years ago by nbruin (previous) (diff)

Changed 6 years ago by nbruin

ensure that SymbolicConstant? object convert into SR

comment:3 Changed 6 years ago by nbruin

  • Status changed from new to needs_review

comment:4 Changed 6 years ago by nbruin

Note that once you get a characteristic polynomial, it's likely wrong. See #14403

comment:5 Changed 6 years ago by eviatarbach

Great! The patch works for me, and all tests pass on constants_c.pyx and constants.py.

Last edited 6 years ago by eviatarbach (previous) (diff)

comment:6 follow-up: Changed 6 years ago by eviatarbach

Maybe it would be good to add doctests for matrix([pi]).charpoly() and SR(pi.pyobject())?

comment:7 in reply to: ↑ 6 Changed 6 years ago by kcrisman

  • Authors set to Nils Bruin
  • Reviewers set to Eviatar Bach
  • Status changed from needs_review to needs_work

Maybe it would be good to add doctests for matrix([pi]).charpoly() and SR(pi.pyobject())?

Agreed.

comment:8 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:9 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:10 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:11 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:12 Changed 4 years ago by vdelecroix

  • Milestone changed from sage-6.4 to sage-6.6

Just works fine on 6.6-rc2

sage: sage: matrix([[sqrt(2), -1], [pi, e^2]]).charpoly()
x^2 + (-sqrt(2) - e^2)*x + pi + sqrt(2)*e^2

Migt be because symbolic constants are now element of the symbolic ring:

sage: type(pi)
<type 'sage.symbolic.expression.Expression'>
sage: pi.pyobject()
pi
sage: type(_)
<class 'sage.symbolic.constants.Pi'>

Vincent

comment:13 Changed 4 years ago by vdelecroix

  • Authors changed from Nils Bruin to Vincent Delecroix
  • Branch set to public/13711
  • Commit set to 005cd02119165e28b2c7a9ae784e447289cfe859
  • Description modified (diff)
  • Reviewers Eviatar Bach deleted
  • Status changed from needs_work to needs_review

New commits:

005cd02Trac 13711: add a doctest to the method charpoly

comment:14 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:15 Changed 4 years ago by vbraun

  • Branch changed from public/13711 to 005cd02119165e28b2c7a9ae784e447289cfe859
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.