Clarify, fix and test semantics of Quotient(R,a,b) #3612
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fingolfin
added
kind: bug
wilfwilson
reviewed
Aug 26, 2019
tst/testinstall/opers/Quotient.tst
Outdated
| <monoid>[t] | ||
| gap> Quotient(2*t, t); | ||
| ZmodnZObj(2,6) | ||
| gap> #Quotient(t, 2*t); # FIXME: should return fail |
There was a problem hiding this comment.
Do you intend to fix this FIXME in this PR?
There was a problem hiding this comment.
@wilfwilson Nope, that would require substantial changes to the polynomial code, which isn't something I plan to look at any time soon.
There was a problem hiding this comment.
I tried to clarify the comment to make it clearer what is going on. Also tried to fix the test failure.
fingolfin
force-pushed
the
mh/Quotient-and-QUO-semantics
branch
from
6676af2
to
a7f3eb7
Compare
Codecov Report
@@ Coverage Diff @@
## master #3612 +/- ##
=========================================
Coverage ? 80.91%
=========================================
Files ? 644
Lines ? 317116
Branches ? 0
=========================================
Hits ? 256607
Misses ? 60509
Partials ? 0
|
wilfwilson
approved these changes
Aug 30, 2019
This improves the documentation of Quotient to make it sensible for non-commutative rings, and rings with zero divisors; it also fixes some methods for Quotient which violated the documentation of Quotient (before and after my changes to it). In particular Quotient(R, x, Zero(R)) should return fail, not run into an error. Also add some tests for various Quotient methods. It turns out that those for polynomials still are broken, which should be fixed in the future.
fingolfin
force-pushed
the
mh/Quotient-and-QUO-semantics
branch
from
a7f3eb7
to
987f6ad
Compare
dimpase
added
release notes: added
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This improves the documentation of
Quotientto make it sensible for non-commutative rings, and rings with zero divisors; it also fixes some methods forQuotientwhich violated the documentation ofQuotient(before and after my changes to it).In particular
Quotient(R, x, Zero(R))should return fail, not run into an error.Also add some tests for various
Quotientmethods. It turns out that those forpolynomials still are broken, which should be fixed in the future.