Opened 8 years ago

Closed 8 years ago

#13692 closed defect (fixed)

factor_trial_division returns Python longs as exponents

Reported by: dimpase Owned by: was
Priority: major Milestone: sage-5.5
Component: number theory Keywords:
Cc: Merged in: sage-5.5.beta2
Authors: Nils Bruin Reviewers: Dmitrii Pasechnik
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by dimpase)

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

Attachments (1)

trac_13692-no_unsigned_long.patch (989 bytes) - added by nbruin 8 years ago.
fix

Download all attachments as: .zip

Change History (7)

comment:1 Changed 8 years ago by dimpase

  • Description modified (diff)

comment:2 follow-up: 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

Replying to nbruin:

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 

Changed 8 years ago by nbruin

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.