Opened 6 years ago

Closed 11 months ago

#21025 closed defect (fixed)

IntegerModRing should coerce (not convert) order

Reported by: jdemeyer Owned by:
Priority: minor Milestone: sage-9.5
Component: basic arithmetic Keywords:
Cc: tscrim, slelievre, gh-kliem Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 7ab4221 (Commits, GitHub, GitLab) Commit: 7ab4221cd73e47db90a352ae46c13b6781f318e8
Dependencies: Stopgaps:

Status badges

Description

The following is nonsense: 

sage: IntegerModRing(Mod(4,5))
Ring of integers modulo 4

Change History (11)

comment:1 Changed 12 months ago by chapoton

  • Branch set to u/chapoton/21025
  • Commit set to bb0b8ce8988152849977c5122f15a6e6c6a18f41
  • Status changed from new to needs_review

New commits:

​bb0b8cecoerce order to ZZ in integer mod ring

comment:2 Changed 12 months ago by git

  • Commit changed from bb0b8ce8988152849977c5122f15a6e6c6a18f41 to 9f1e111f32cbf879f03028dce2bcf29785742f93

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

​9f1e111one fix

comment:3 Changed 12 months ago by chapoton

  • Milestone changed from sage-7.3 to sage-9.5

comment:4 Changed 12 months ago by git

  • Commit changed from 9f1e111f32cbf879f03028dce2bcf29785742f93 to c117d1567c2b5bfb2d07b261b9f71f735b1cbe2a

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

​c117d15another fix

comment:5 Changed 12 months ago by git

  • Commit changed from c117d1567c2b5bfb2d07b261b9f71f735b1cbe2a to fea3386e8b75357561e9c5b58f151c9212a485ae

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​fea3386coerce order to ZZ in integer mod ring

comment:6 Changed 12 months ago by chapoton

  • Authors set to Frédéric Chapoton
  • Cc tscrim slelievre gh-kliem added

green bot, please review

comment:7 Changed 12 months ago by tscrim

This is a problem: 

sage: ZZ.coerce(4/2)
...
TypeError: no canonical coercion from Rational Field to Integer Ring

It is very likely that a user can accidentally have a rational, which looks like an integer, but it is not. Here, I think it should work smoothly for the rational case. We can special case this, but what about if we take a constant polynomial ring over ZZ (or over QQ)? That case is a lot more unlikely, but still within the realm possibility. I am leaning towards an overall -1 on this change. I don't quite see the argument why conversion is so bad as it defines what can be considered an integer. (E.g., ZZ.coerce('2') also doesn't work.)

comment:8 Changed 12 months ago by git

  • Commit changed from fea3386e8b75357561e9c5b58f151c9212a485ae to 7ab4221cd73e47db90a352ae46c13b6781f318e8

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​8a88516some fixes about use of IntegerModRing
​7ab4221pep8 cleanup of integer_mod_ring.py

comment:9 Changed 12 months ago by chapoton

Jeroen is probably no longer around to defend his point of view.

Then let us keep only the non-controversial fixes where this was called in the code with non-integer arguments.

I have also pep8-cleaned the file integer_mod_ring

comment:10 Changed 11 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

That works for me. Thanks.

comment:11 Changed 11 months ago by vbraun

  • Branch changed from u/chapoton/21025 to 7ab4221cd73e47db90a352ae46c13b6781f318e8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.