Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix QuotientMod docs, and the integer implementation #1991

Merged
merged 1 commit into from
Dec 10, 2017

Conversation

fingolfin
Copy link
Member

This partially reverts changes we made 2013. The documentation
is now correct (resp. consistent again), and several corner cases
work correctly. E.g. QuotientMod(0,0,m) never worked correctly, even
though by the definition given in the manual, 1 is a valid result.

Fixes #149

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Dec 5, 2017
@codecov
Copy link

codecov bot commented Dec 6, 2017

Codecov Report

Merging #1991 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1991      +/-   ##
==========================================
+ Coverage   65.94%   65.95%   +<.01%     
==========================================
  Files         898      898              
  Lines      273206   273679     +473     
  Branches    12771    11949     -822     
==========================================
+ Hits       180175   180513     +338     
- Misses      90211    90312     +101     
- Partials     2820     2854      +34
Impacted Files Coverage Δ
lib/integer.gi 78.1% <100%> (+0.58%) ⬆️
hpcgap/lib/integer.gi 63.88% <100%> (+0.88%) ⬆️
lib/queue.g 66.4% <0%> (-3.2%) ⬇️
src/hpc/thread.c 45.41% <0%> (-1%) ⬇️
src/funcs.c 76.24% <0%> (-0.27%) ⬇️
src/objset.c 82.59% <0%> (-0.24%) ⬇️
src/hpc/threadapi.c 34.64% <0%> (-0.1%) ⬇️
src/stats.c 79.3% <0%> (ø) ⬆️
src/streams.c 47.58% <0%> (+0.42%) ⬆️
hpcgap/lib/zmodnz.gi 94.92% <0%> (+1.08%) ⬆️
... and 7 more

@fingolfin
Copy link
Member Author

@alex-konovalov this is reverting resp. adjusting some changes you made back then, so perhaps you have some thoughts on this?

@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Dec 9, 2017
@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Dec 10, 2017

For example, what's changed:

  1. In GAP 4.8.8, both QuotientMod(4, 2, 6); and QuotientMod(2, 4, 6); return fail. With this PR, the former is 2, the latter is fail.

  2. for a := ZmodnZObj(2, 6); and b := ZmodnZObj(4, 6);, both b/a and a/b return fail in GAP 4.8.8. With this PR, we have

gap> b/a;
ZmodnZObj( 2, 6 )
gap> a/b;
fail

Shouldn't both cases in the 2nd example return a non-fail result? In the first example we have a Euclidean ring, and its new output now matches the documentation, so fine. But what about the 2nd example?

Otherwise, I ran GAP test suite in Jenkins for this PR, and did not discover any regressions.

@fingolfin
Copy link
Member Author

@alex-konovalov you are 100% right, I am glad you paid attention and double checked! In fact, also the result for QuotientMod(0,0,m) was wrong -- it must be 0, per the documentation (which apparently I did not read carefully enough sigh).

All of this should be resolved now. Also, I changed the code to use INVMODINT instead of going through mod for rationals. I.e. a tiny performance improvement.

This partially reverts changes we made 2013. The documentation
is now correct (resp. consistent again), and several corner cases
work correctly. E.g.  QuotientMod(0,0,m) never worked correctly, even
though by the definition given in the manual, it should return 0.

Fixes gap-system#149
Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fingolfin - now looks good, and I've rerun Jenkins test to see if it has any impact on package tests - apparently none.

@olexandr-konovalov olexandr-konovalov merged commit a57e49b into gap-system:master Dec 10, 2017
@fingolfin fingolfin deleted the mh/QuotientMod branch December 11, 2017 01:26
@mohamed-barakat
Copy link
Member

Excellent, thank you both very much.

@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants