# Ticket #10064(positive_review defect)

Opened 3 years ago

## -1 in expression "is_positive".

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

### Description (last modified by burcin) (diff)

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

## Change History

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

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

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 3 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 3 years ago by jgaski

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

### comment:5 Changed 3 years ago by kcrisman

• Cc kcrisman added

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

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

### comment:7 Changed 17 months ago by mhansen

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

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

Apply trac_10064.patch

### comment:8 Changed 17 months ago by kcrisman

• Dependencies changed from 7160 to #7160

### comment:9 Changed 17 months ago by mhansen

• Keywords sd35 added

### comment:10 Changed 17 months ago by burcin

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

Looks good to me.

### comment:11 Changed 17 months ago by jdemeyer

• Milestone changed from sage-5.0 to sage-pending
Note: See TracTickets for help on using tickets.