Opened 8 years ago

Closed 8 years ago

# factor_trial_division returns Python longs as exponents

Reported by: Owned by: dimpase was major sage-5.5 number theory sage-5.5.beta2 Nils Bruin Dmitrii Pasechnik N/A

unlike the rest (?) of Sage, factor_trial_division returns exponents of primes as Python longs:

```sage: from sage.rings.factorint import factor_trial_division
sage: [t for t in factor_trial_division(8)]
[(2, 3L)]
```

It is a bug or a feature? It leads to this kind of data also being returned by factor():

```sage: [t for t in Integer(8).factor(limit=1000)]
[(2, 3L)]
```

as opposed to

```sage: [t for t in Integer(8).factor()]
[(2, 3)]
```

See this sage-support thread. Something should be fixed - either factor(), or factor_trial_division()

Apply

### comment:1 Changed 8 years ago by dimpase

• Description modified (diff)

### comment:2 follow-up: ↓ 3 Changed 8 years ago by nbruin

• Authors set to Nils Bruin
• Status changed from new to needs_review

Problem was that exponent was a `cdef unsigned long`, which doesn't necessarily fit in a python int, so apparently cython converts that to a python long automatically.

By defining it to be a `cdef long` instead I don't think we'll get any overflow (it's an exponent of a prime in the factorization of an integer!) and now the exponent is returned as a `python int` instead. Unless people feel strongly that this should be cast to a `Integer` instead, I think this should be sufficient.

### comment:3 in reply to: ↑ 2 Changed 8 years ago by dimpase

• Description modified (diff)
• Reviewers set to Dmitrii Pasechnik
• Status changed from needs_review to positive_review

Problem was that exponent was a `cdef unsigned long`, which doesn't necessarily fit in a python int, so apparently cython converts that to a python long automatically.

By defining it to be a `cdef long` instead I don't think we'll get any overflow (it's an exponent of a prime in the factorization of an integer!) and now the exponent is returned as a `python int` instead. Unless people feel strongly that this should be cast to a `Integer` instead, I think this should be sufficient.

To have the exponents of type int() is consistent, it seem. Looks good, positive review.

### comment:4 Changed 8 years ago by jdemeyer

• Status changed from positive_review to needs_work

The documentation is misformatted. It should be

```TESTS:

Test that :trac:`13692` is solved::

sage: from sage.rings.factorint import factor_trial_division
```

fix

### comment:5 Changed 8 years ago by nbruin

• Status changed from needs_work to positive_review

Test section formatting amended

### comment:6 Changed 8 years ago by jdemeyer

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