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 )
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)
Change History (7)
comment:1 Changed 8 years ago by
- Description modified (diff)
comment:2 follow-up: ↓ 3 Changed 8 years ago by
- Status changed from new to needs_review
comment:3 in reply to: ↑ 2 Changed 8 years ago by
- 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 apython int
instead. Unless people feel strongly that this should be cast to aInteger
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
- 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
comment:5 Changed 8 years ago by
- Status changed from needs_work to positive_review
Test section formatting amended
comment:6 Changed 8 years ago by
- Merged in set to sage-5.5.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
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 apython int
instead. Unless people feel strongly that this should be cast to aInteger
instead, I think this should be sufficient.