Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#18332 closed enhancement (fixed)

is_one/is_integer/is_rational for number field elements

Reported by: Vincent Delecroix Owned by:
Priority: major Milestone: sage-6.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:

Status badges

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 Vincent Delecroix

Branch: u/vdelecroix/18332
Commit: e08010c103701ba560d0a31432fd67ed7b7de958
Status: newneeds_review

New commits:

e08010cTrac 18332: is_integer and is_rational for number field elements

comment:2 Changed 7 years ago by Marc Mezzarobba

Hi Vincent,

Is there a reason to keep the old is_rational_c() methods?

comment:3 Changed 7 years ago by git

Commit: e08010c103701ba560d0a31432fd67ed7b7de958465271f1e9f0fa42b72cd050d590ea325d4175d6

Branch pushed to git repo; I updated commit sha1. New commits:

465271fTrac 18332: remove is_rational_c, add is_one

comment:4 in reply to:  2 ; Changed 7 years ago by Vincent Delecroix

Summary: is_integer/is_rational for number field elementsis_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 ; Changed 7 years ago by Marc Mezzarobba

Replying to vdelecroix:

No. Actually, this method was wrong since it returned False for 0...

Yes, thoug it didn't matter in the only place where it was used :-)

I added a method is_one and rewrote multiplicative_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 Vincent Delecroix

Replying to mmezzarobba:

Replying to vdelecroix:

I added a method is_one and rewrote multiplicative_order.

But then I guess you'll want to declare is_rational and is_one as cpdef.

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 git

Commit: 465271f1e9f0fa42b72cd050d590ea325d4175d68fa6326cc34cdb44566758832111cbbc3542f46a

Branch pushed to git repo; I updated commit sha1. New commits:

8fa6326Trac 18332: two def -> cpdef + documentation

comment:8 Changed 7 years ago by Marc Mezzarobba

Reviewers: Marc Mezzarobba
Status: needs_reviewpositive_review

comment:9 Changed 7 years ago by Volker Braun

Branch: u/vdelecroix/183328fa6326cc34cdb44566758832111cbbc3542f46a
Resolution: fixed
Status: positive_reviewclosed

comment:10 Changed 6 years ago by Peleg Michaeli

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.

Note: See TracTickets for help on using tickets.