#18332 closed enhancement (fixed)
is_one/is_integer/is_rational for number field elements
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage6.7 
Component:  number fields  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Marc Mezzarobba 
Report Upstream:  N/A  Work issues:  
Branch:  8fa6326 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
We add two methods is_integer
and is_rational
for number field elements.
Currently, the only way to test this is to do duck typing
def is_integer(x): try: ZZ(x) return True except ValueError: return False
which is rather expensive compared to what can be done.
Change History (10)
comment:1 Changed 7 years ago by
 Branch set to u/vdelecroix/18332
 Commit set to e08010c103701ba560d0a31432fd67ed7b7de958
 Status changed from new to needs_review
comment:2 followup: ↓ 4 Changed 7 years ago by
Hi Vincent,
Is there a reason to keep the old is_rational_c()
methods?
comment:3 Changed 7 years ago by
 Commit changed from e08010c103701ba560d0a31432fd67ed7b7de958 to 465271f1e9f0fa42b72cd050d590ea325d4175d6
Branch pushed to git repo; I updated commit sha1. New commits:
465271f  Trac 18332: remove is_rational_c, add is_one

comment:4 in reply to: ↑ 2 ; followup: ↓ 5 Changed 7 years ago by
 Summary changed from is_integer/is_rational for number field elements to is_one/is_integer/is_rational for number field elements
Replying to mmezzarobba:
Hi Vincent,
Is there a reason to keep the old
is_rational_c()
methods?
No. Actually, this method was wrong since it returned False
for 0
... I added a method is_one
and rewrote multiplicative_order
.
comment:5 in reply to: ↑ 4 ; followup: ↓ 6 Changed 7 years ago by
Replying to vdelecroix:
No. Actually, this method was wrong since it returned
False
for0
...
Yes, thoug it didn't matter in the only place where it was used :)
I added a method
is_one
and rewrotemultiplicative_order
.
But then I guess you'll want to declare is_rational
and is_one
as cpdef
.
comment:6 in reply to: ↑ 5 Changed 7 years ago by
Replying to mmezzarobba:
Replying to vdelecroix:
I added a method
is_one
and rewrotemultiplicative_order
.But then I guess you'll want to declare
is_rational
andis_one
ascpdef
.
Do you think so? In sage.structure.RingElement
the is_one
method is declared as def
with the following default implementation
def is_one(self): return self == self._parent.one()
But it is true that it might be relatively intensively used in number fields. I will do that.
comment:7 Changed 7 years ago by
 Commit changed from 465271f1e9f0fa42b72cd050d590ea325d4175d6 to 8fa6326cc34cdb44566758832111cbbc3542f46a
Branch pushed to git repo; I updated commit sha1. New commits:
8fa6326  Trac 18332: two def > cpdef + documentation

comment:8 Changed 7 years ago by
 Reviewers set to Marc Mezzarobba
 Status changed from needs_review to positive_review
comment:9 Changed 7 years ago by
 Branch changed from u/vdelecroix/18332 to 8fa6326cc34cdb44566758832111cbbc3542f46a
 Resolution set to fixed
 Status changed from positive_review to closed
comment:10 Changed 5 years ago by
 Commit 8fa6326cc34cdb44566758832111cbbc3542f46a deleted
Why won't we add these methods to expressions as well?
Integer
, Rational
and AlgebraicNumber
have is_integer
, is_one
and is_zero
but not is_real
or is_rational
or is_algebraic
.
New commits:
Trac 18332: is_integer and is_rational for number field elements