Opened 13 months ago
Closed 12 months ago
#28967 closed enhancement (fixed)
implement easy cases of inverse_mod for general commutative rings
Reported by:  ghDaveWitteMorris  Owned by:  

Priority:  minor  Milestone:  sage9.1 
Component:  commutative algebra  Keywords:  
Cc:  Merged in:  
Authors:  Dave Morris  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  4db4569 (Commits)  Commit:  4db45698a8a3e554a7eadf95aab3951309eab5da 
Dependencies:  #28969  Stopgaps: 
Description
The definition of inverse_mod(x, I)
for the class CommutativeRingElement
always raises a NotImplementedError
. It would be better to return the correct value in the easy cases (when x
is a unit, or x
is in I
). This would provide a complete implementation for fields.
Change History (13)
comment:1 Changed 13 months ago by
 Branch set to public/28967
comment:2 Changed 13 months ago by
 Commit set to 139c8d2a71f849e1ccdd10a8dbd9d67cc193d4b8
 Status changed from new to needs_review
comment:3 Changed 13 months ago by
 Status changed from needs_review to needs_work
Failed doctest (because I fixed a typo and didn't notice it was in a doctest). Also, probably should have a better error message for commutative rings that do not implement is_unit
.
comment:4 Changed 13 months ago by
 Commit changed from 139c8d2a71f849e1ccdd10a8dbd9d67cc193d4b8 to f312d7c3b42c0815271e08e7a97f5880b8267a7d
comment:5 Changed 13 months ago by
 Dependencies set to #28969
 Status changed from needs_work to needs_review
Slightly improved the handling of is_unit
, and changed the ValueError
message to match the one given by univariate polynomial rings. Ticket #28969 should fix the doctest error.
comment:6 Changed 13 months ago by
The error message Impossible inverse modulo
is a little strange to me, and moreover from the ticket description, it does not seem correct. Could we instead change it to
raise NotImplementedError("unable to compute the inverse modulo this ideal")
On #28969, it would have been good to also remove that period at the end of it in order to follow Python's error message convention. Oh well, that already has a positive review...
comment:7 Changed 13 months ago by
 Commit changed from f312d7c3b42c0815271e08e7a97f5880b8267a7d to e2a84fab1a646c82539f559323e2603219c227ef
Branch pushed to git repo; I updated commit sha1. New commits:
e2a84fa  change error message

comment:8 Changed 13 months ago by
Being a beginner at sagemath development, I thought I should copy an error message, instead of making up my own, but, since you agree that it is weird, I will change it. However, this is a ValueError
, not a NotImplementedError
, because the inverse does not exist. (This error message is for the case where x is in I and I is not the entire ring. Since x is in I, we have <x,I> = I. Also, if the inverse of x modulo I exists, then <x,I> must be the entire ring. So I = <x,I> is the entire ring, which is a contradiction.) I changed the error message to
raise ValueError("An element of a proper ideal does not have an inverse modulo that ideal")
New commits:
e2a84fa  change error message

comment:9 Changed 13 months ago by
 Component changed from linear algebra to commutative algebra
 Reviewers set to Travis Scrimshaw
I see, I misunderstood the case. If you could just change the error message to start with a lowercase letter (this is a general Python convention we follow), then you can set a positive review.
comment:10 Changed 13 months ago by
 Commit changed from e2a84fab1a646c82539f559323e2603219c227ef to 4db45698a8a3e554a7eadf95aab3951309eab5da
Branch pushed to git repo; I updated commit sha1. New commits:
4db4569  error message should be lowercase

comment:11 Changed 13 months ago by
Thanks for the comments and corrections. I'll try to write error messages correctly in the future.
comment:13 Changed 12 months ago by
 Branch changed from public/28967 to 4db45698a8a3e554a7eadf95aab3951309eab5da
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
implement basic functionality