Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#18332 closed enhancement (fixed)

is_one/is_integer/is_rational for number field elements

Reported by: vdelecroix 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 vdelecroix

  • Branch set to u/vdelecroix/18332
  • Commit set to e08010c103701ba560d0a31432fd67ed7b7de958
  • Status changed from new to needs_review

New commits:

e08010cTrac 18332: is_integer and is_rational for number field elements

comment:2 follow-up: Changed 7 years ago by mmezzarobba

Hi Vincent,

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

comment:3 Changed 7 years ago by git

  • Commit changed from e08010c103701ba560d0a31432fd67ed7b7de958 to 465271f1e9f0fa42b72cd050d590ea325d4175d6

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 ; follow-up: Changed 7 years ago by vdelecroix

  • 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 ; follow-up: Changed 7 years ago by mmezzarobba

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 vdelecroix

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 changed from 465271f1e9f0fa42b72cd050d590ea325d4175d6 to 8fa6326cc34cdb44566758832111cbbc3542f46a

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

8fa6326Trac 18332: two def -> cpdef + documentation

comment:8 Changed 7 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

comment:9 Changed 7 years ago by vbraun

  • 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 pelegm

  • 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.

Note: See TracTickets for help on using tickets.