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:  sage9.5 
Component:  basic arithmetic  Keywords:  
Cc:  tscrim, slelievre, ghkliem  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: 
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
 Branch set to u/chapoton/21025
 Commit set to bb0b8ce8988152849977c5122f15a6e6c6a18f41
 Status changed from new to needs_review
comment:2 Changed 12 months ago by
 Commit changed from bb0b8ce8988152849977c5122f15a6e6c6a18f41 to 9f1e111f32cbf879f03028dce2bcf29785742f93
Branch pushed to git repo; I updated commit sha1. New commits:
9f1e111  one fix

comment:3 Changed 12 months ago by
 Milestone changed from sage7.3 to sage9.5
comment:4 Changed 12 months ago by
 Commit changed from 9f1e111f32cbf879f03028dce2bcf29785742f93 to c117d1567c2b5bfb2d07b261b9f71f735b1cbe2a
Branch pushed to git repo; I updated commit sha1. New commits:
c117d15  another fix

comment:5 Changed 12 months ago by
 Commit changed from c117d1567c2b5bfb2d07b261b9f71f735b1cbe2a to fea3386e8b75357561e9c5b58f151c9212a485ae
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fea3386  coerce order to ZZ in integer mod ring

comment:7 Changed 12 months ago by
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
 Commit changed from fea3386e8b75357561e9c5b58f151c9212a485ae to 7ab4221cd73e47db90a352ae46c13b6781f318e8
comment:9 Changed 12 months ago by
Jeroen is probably no longer around to defend his point of view.
Then let us keep only the noncontroversial fixes where this was called in the code with noninteger arguments.
I have also pep8cleaned the file integer_mod_ring
comment:10 Changed 11 months ago by
 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
 Branch changed from u/chapoton/21025 to 7ab4221cd73e47db90a352ae46c13b6781f318e8
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
coerce order to ZZ in integer mod ring