Opened 9 years ago

Closed 6 years ago

#10064 closed defect (fixed)

-1 in expression "is_positive".

Reported by: fmaltey Owned by: burcin
Priority: critical Milestone: sage-5.11
Component: calculus Keywords: sd35
Cc: robertwb, kcrisman Merged in: sage-5.11.beta1
Authors: Mike Hansen Reviewers: Burcin Erocal
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #7160 Stopgaps:

Description (last modified by burcin)

I test :

y = I*I*x / x # so y is the expression -1
y._is_positive()  # the answer is True (and I dislike)
z = -x / x 
z._is_positive()   # the answer is False 
bool (z == y) # the answer is True

Apply trac_10064.sage-5.10.beta5.patch.

Attachments (2)

trac_10064.patch (957 bytes) - added by mhansen 8 years ago.
trac_10064.sage-5.10.beta5.patch (956 bytes) - added by burcin 6 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by burcin

  • Milestone set to sage-4.6

Simpler example:

sage: (I*I)._is_positive()
True
sage: I*I
-1

The is_positive() test in pynac is at line 923 in numeric.cpp:

    case PYOBJECT:
      n = is_real() && (PyObject_Compare(v._pyobject, ZERO) > 0);
      if (PyErr_Occurred()) 
	py_error("is_positive");
      return n;

We use the python comparison. Unfortunately, this doesn't work as expected for number field elements:

sage: m = (I*I).pyobject()
sage: m.parent()
Number Field in I with defining polynomial x^2 + 1
sage: m > 0
True
sage: m
-1

comment:2 Changed 9 years ago by burcin

  • Authors set to Burcin Erocal
  • Cc robertwb added
  • Priority changed from major to critical
  • Status changed from new to needs_review

attachment:trac_10064-compare_I.patch changes the quadratic number field element comparison code to return a more sensible result. Now we have:

sage: m = (I*I).pyobject()
sage: m > 0
False
sage: m < 0
True

This also fixes #10062.

comment:3 Changed 9 years ago by jgaski

  • Status changed from needs_review to needs_work

Here is a small bit of code that breaks (in 4.5.3) with trac_10064-compare_I.patch applied:

K.<a> = QuadraticField(5)
E = EllipticCurve(K,[-1, -1, 1/2*a + 1/2, -1/2*a - 1/2, 1/2*a + 1/2])
E.torsion_subgroup()

comment:4 Changed 9 years ago by jgaski

Please see http://tinypaste.com/3f27d for the error from the above code snippet.

comment:5 Changed 9 years ago by kcrisman

  • Cc kcrisman added

comment:6 Changed 9 years ago by kcrisman

Apparently related to #7160 and #6132, see this sage-devel discussion.

Changed 8 years ago by mhansen

comment:7 Changed 8 years ago by mhansen

  • Authors changed from Burcin Erocal to Mike Hansen
  • Dependencies set to 7160
  • Status changed from needs_work to needs_review

I've added a patch which verifies that is fixed by #7160.

Apply trac_10064.patch

comment:8 Changed 8 years ago by kcrisman

  • Dependencies changed from 7160 to #7160

comment:9 Changed 8 years ago by mhansen

  • Keywords sd35 added

comment:10 Changed 8 years ago by burcin

  • Description modified (diff)
  • Milestone changed from sage-4.8 to sage-5.0
  • Reviewers set to Burcin Erocal
  • Status changed from needs_review to positive_review

Looks good to me.

comment:11 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.0 to sage-pending

Changed 6 years ago by burcin

comment:12 Changed 6 years ago by burcin

  • Description modified (diff)
  • Milestone changed from sage-pending to sage-5.11

Now that #7160 is fixed (by way of #13213), this can be merged.

comment:13 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.11.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.