#28967 closed enhancement (fixed)

implement easy cases of inverse_mod for general commutative rings

Reported by: gh-DaveWitteMorris Owned by:
Priority: minor Milestone: sage-9.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 gh-DaveWitteMorris

  • Branch set to public/28967

comment:2 Changed 13 months ago by gh-DaveWitteMorris

  • Authors set to Dave Morris
  • Commit set to 139c8d2a71f849e1ccdd10a8dbd9d67cc193d4b8
  • Status changed from new to needs_review

New commits:

139c8d2implement basic functionality

comment:3 Changed 13 months ago by gh-DaveWitteMorris

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

  • Commit changed from 139c8d2a71f849e1ccdd10a8dbd9d67cc193d4b8 to f312d7c3b42c0815271e08e7a97f5880b8267a7d

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

46e1047better handling of is_unit
a817901fix typo in error message
587652calso correct file element.pyx
f312d7cmerge ticket 28969 to fix doctest

comment:5 Changed 13 months ago by gh-DaveWitteMorris

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

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 git

  • Commit changed from f312d7c3b42c0815271e08e7a97f5880b8267a7d to e2a84fab1a646c82539f559323e2603219c227ef

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

e2a84fachange error message

comment:8 Changed 13 months ago by gh-DaveWitteMorris

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:

e2a84fachange error message

comment:9 Changed 13 months ago by tscrim

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

  • Commit changed from e2a84fab1a646c82539f559323e2603219c227ef to 4db45698a8a3e554a7eadf95aab3951309eab5da

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

4db4569error message should be lowercase

comment:11 Changed 13 months ago by gh-DaveWitteMorris

Thanks for the comments and corrections. I'll try to write error messages correctly in the future.

comment:12 Changed 13 months ago by tscrim

  • Status changed from needs_review to positive_review

Thanks.

comment:13 Changed 12 months ago by vbraun

  • Branch changed from public/28967 to 4db45698a8a3e554a7eadf95aab3951309eab5da
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.