#18332 closed enhancement (fixed)
is_one/is_integer/is_rational for number field elements
Reported by:  Vincent Delecroix  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:  → u/vdelecroix/18332 

Commit:  → e08010c103701ba560d0a31432fd67ed7b7de958 
Status:  new → 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:  e08010c103701ba560d0a31432fd67ed7b7de958 → 465271f1e9f0fa42b72cd050d590ea325d4175d6 

Branch pushed to git repo; I updated commit sha1. New commits:
465271f  Trac 18332: remove is_rational_c, add is_one

comment:4 followup: 5 Changed 7 years ago by
Summary:  is_integer/is_rational for number field elements → 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 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 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:  465271f1e9f0fa42b72cd050d590ea325d4175d6 → 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:  → Marc Mezzarobba 

Status:  needs_review → positive_review 
comment:9 Changed 7 years ago by
Branch:  u/vdelecroix/18332 → 8fa6326cc34cdb44566758832111cbbc3542f46a 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:10 Changed 6 years ago by
Commit:  8fa6326cc34cdb44566758832111cbbc3542f46a 

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