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

Fixed RandomUnimodularMat for m=1 case #1508

Closed
wants to merge 1 commit into from
Closed

Conversation

wagh
Copy link
Contributor

@wagh wagh commented Jul 16, 2017

Otherwise it goes in an infinite loop.

Otherwise it goes in an infinite loop.
@codecov
Copy link

codecov bot commented Jul 16, 2017

Codecov Report

Merging #1508 into master will decrease coverage by <.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1508      +/-   ##
==========================================
- Coverage   64.17%   64.17%   -0.01%     
==========================================
  Files         992      992              
  Lines      322141   322144       +3     
  Branches    13087    13090       +3     
==========================================
- Hits       206749   206748       -1     
- Misses     112570   112577       +7     
+ Partials     2822     2819       -3
Impacted Files Coverage Δ
lib/matrix.gi 76.23% <66.66%> (-0.02%) ⬇️
src/system.c 53.03% <0%> (-0.85%) ⬇️
lib/init.g 45.65% <0%> (-0.28%) ⬇️
src/listfunc.c 76.77% <0%> (-0.18%) ⬇️
src/vecgf2.c 56.82% <0%> (-0.11%) ⬇️
src/hpc/threadapi.c 31.69% <0%> (+0.09%) ⬆️
src/stats.c 72.66% <0%> (+0.13%) ⬆️
src/objset.c 93.31% <0%> (+0.26%) ⬆️
src/hpc/traverse.c 79.77% <0%> (+0.38%) ⬆️
src/hpc/thread.c 46.64% <0%> (+0.39%) ⬆️

@ChrisJefferson
Copy link
Contributor

Can you add a test case? While writing one, feel free to test some other cases of the function, if you find there are no tests for it :) (adding more tests is not required!)

@ChrisJefferson
Copy link
Contributor

Also don't worry about the failing tests from Travis, there is currently a bug in Travis making 32 bit gap builds fail.

@wagh
Copy link
Contributor Author

wagh commented Jul 16, 2017

Some test cases:

gap> RandomUnimodularMat( 3 );
[ [ 0, 1, -6 ], [ 1, -2, 12 ], [ -2, 7, -43 ] ]

(as expected answer)

gap> RandomUnimodularMat( 1 );

is never ending...

But after the patch,

gap> RandomUnimodularMat( 1 );
[ [ -1 ] ]

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Jul 16, 2017
@fingolfin
Copy link
Member

@wagh Thanks for not only reporting this, but also providing a fix. I have taken your commit, and added a test case, plus some extra work, in PR #1511 -- if you are curious, you can look there to learn how to turn a manual test case into one that is run as part of our automated test suite.

If the tests there pass, we can merge it, and close both PRs.

@wagh
Copy link
Contributor Author

wagh commented Jul 17, 2017

Thanks Max. Next time, I will try to create a test-suite!

@markuspf
Copy link
Member

This has been superseded by #1511

@markuspf markuspf closed this Jul 17, 2017
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants