Opened 6 years ago

Closed 6 years ago

#5047 closed defect (fixed)

[with patch, positive review] comparing complex i raises error

Reported by: burcin Owned by: somebody
Priority: major Milestone: sage-3.3
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

sage: cmp(i,0)

TypeError                                 Traceback (most recent call last)
/home/burcin/sage/sage-3.2.3/local/lib/python2.5/site-packages/sage/functions/functions.pyc in __cmp__(self, right)
    267             return 0
    268         R = RealField()
--> 269         c = cmp(R(self), R(right))
    270         if c: return c
    271         try:

...
/home/burcin/sage/sage-3.2.3/local/lib/python2.5/site-packages/sage/functions/constants.pyc in _mpfr_(self, R)
    865             TypeError
    866         """
--> 867         raise TypeError
    868 
    869     def _real_rqdf_(self, R):

TypeError: 

Attachments (2)

cmp_imaginary.patch (852 bytes) - added by SimonKing 6 years ago.
raise a proper error message when using cmp on non-real constants
cmp_doc.patch (1.1 KB) - added by SimonKing 6 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 follow-up: Changed 6 years ago by SimonKing

I believe this ticket is close to being invalid, because Sage is nearly consistent with Python.

In Python, one gets

>>> cmp(1j,0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: no ordering relation is defined for complex numbers

Only I would say that the error message in Sage could be clearer. So, I suggest to catch the error and reraise a TypeError? with an appropriate error message.

Changed 6 years ago by SimonKing

raise a proper error message when using cmp on non-real constants

comment:2 in reply to: ↑ 1 Changed 6 years ago by SimonKing

  • Summary changed from comparing complex i raises error to [with patch, needs review] comparing complex i raises error

Replying to SimonKing:

In Python, one gets

>>> cmp(1j,0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: no ordering relation is defined for complex numbers

With the patch, one gets

sage: cmp(i,0)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/king/.sage/temp/mpc739/25379/_home_king__sage_init_sage_0.py in <module>()
----> 1 
      2 
      3 
      4 
      5 

/home/king/SAGE/devel/sage-3.2.1/local/lib/python2.5/site-packages/sage/functions/functions.pyc in __cmp__(self, right)
    270             c = cmp(R(self), R(right))
    271         except TypeError:
--> 272             raise TypeError, "these objects are not comparable"
    273         if c: return c
    274         try:

TypeError: these objects are not comparable

which, I believe, is consistent with python.

Other comparisons still work:

sage: cmp(i^2,0)
-1
sage: cmp(e,0)
1

comment:3 follow-up: Changed 6 years ago by burcin

  • Cc was added
  • Summary changed from [with patch, needs review] comparing complex i raises error to [with patch, positive review pending changes] comparing complex i raises error

I ran into this when testing new printing code in pynac. ATM, pynac uses the sign function, which is defined to be cmp(x, 0), to determine if it should print a minus sign. Thus, we have the following:

sage: var('x',ns=1)
sage: i*x
<boom>

I somehow thought that the cmp function was not supposed to raise exceptions, but googling shortly didn't turn up any evidence to support this argument.

I would be ok to live with this fact, and try to figure out a better way to handle the sign function, or printing in pynac.

BTW, the sign function is also mentioned here: #777

I give the attached patch a positive review, provided that doctests are added to test for the new error message.

Changed 6 years ago by SimonKing

comment:4 in reply to: ↑ 3 Changed 6 years ago by SimonKing

The second patch cmp_doc.patch (to be applied after the first patch) adds more documentation.

Replying to burcin:

I ran into this when testing new printing code in pynac. ATM, pynac uses the sign function, which is defined to be cmp(x, 0), to determine if it should print a minus sign. Thus, we have the following:

sage: var('x',ns=1)
sage: i*x
<boom>

Ok, but I think there should be a different approach for determining the sign when printing an imaginary number.
However, personally I believe that sign(I) should raise an error.

I somehow thought that the cmp function was not supposed to raise exceptions, but googling shortly didn't turn up any evidence to support this argument.

... and, as I mentioned above, cmp does raise an exception in Python.

BTW, the sign function is also mentioned here: #777

The sign function defined in #777 would simply return 0 on non-real constants, because it tests bool(x > 0). Indeed, we have

sage: bool(I > 0)
False
sage: bool(I < 0)
False

so, at least it does not go boom (which it should, though!).

comment:5 Changed 6 years ago by burcin

  • Cc was removed
  • Summary changed from [with patch, positive review pending changes] comparing complex i raises error to [with patch, positive review] comparing complex i raises error

Positive review, both patches should be applied.

comment:6 Changed 6 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 3.3.alpha2

Note: See TracTickets for help on using tickets.