Opened 12 years ago

Closed 9 years ago

# -1 in expression "is_positive".

Reported by: Owned by: fmaltey burcin critical sage-5.11 calculus sd35 robertwb, kcrisman sage-5.11.beta1 Mike Hansen Burcin Erocal N/A #7160

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
```

### comment:1 Changed 12 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 12 years ago by burcin

• Authors set to Burcin Erocal
• 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 12 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 12 years ago by jgaski

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

### comment:6 Changed 11 years ago by kcrisman

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

### comment:7 Changed 10 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 10 years ago by kcrisman

• Dependencies changed from 7160 to #7160

### comment:10 Changed 10 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 10 years ago by jdemeyer

• Milestone changed from sage-5.0 to sage-pending

### comment:12 Changed 9 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 9 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.