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 MagmaWith[One|Inverses] "no method found" errors #1798

Merged
merged 3 commits into from
Oct 26, 2017
Merged

Fix MagmaWith[One|Inverses] "no method found" errors #1798

merged 3 commits into from
Oct 26, 2017

Conversation

gwhitney
Copy link

@gwhitney gwhitney commented Oct 23, 2017

The current master version of GAP has the following bug: (otherwise unreported, please let me know if I should have opened an issue before submitting this pull request)

The 2-argument forms of MagmaWithOne and MagmaWithInverses typically do not (perhaps never) succeed as documented, but rather report "no method found" errors. See the following gap transcript for a simple example of this behavior:

gap> Magma(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
<group with 2 generators>
gap> Elements(last);
[ (), (2,3), (1,2), (1,2,3), (1,3,2), (1,3) ]
gap> MagmaWithOne(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `MagmaWithOneByGenerators' on 1 arguments at /home/gwhitney/code/dev/gap/lib/methsel2.g:241 called from
MagmaWithOneByGenerators( arg ) at /home/gwhitney/code/dev/gap/lib/magma.gi:426 called from
<function "MagmaWithOne">( <arguments> )
 called from read-eval loop at *stdin*:3
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> quit;
gap> MagmaWithInverses(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `MagmaWithInversesByGenerators' on 1 arguments at /home/gwhitney/code/dev/gap/lib/methsel2.g:241 called from
MagmaWithInversesByGenerators( arg ) at /home/gwhitney/code/dev/gap/lib/magma.gi:503 called from
<function "MagmaWithInverses">( <arguments> )
 called from read-eval loop at *stdin*:3
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue

Checking the code in lib/magma.gi, the difficulty is clear: in the portions of the code for the "#family plus list of generators" alternative for the arguments to both of these functions, the first argument is correctly checked to be a family, but then the first argument, rather than the second, is checked to be a list. Since no family is a list, that branch of the code is never taken, and it falls through to the "# generators" option for the arguments, eventually failing because a family is not a sensible generator for a magma.

The submitted two-character change corrects the code to check the second argument in these situations for being a list. The following log shows the transcript of the same commands executed on the corrected code; they succeed as expected:

gap> MagmaWithOne(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
Group([ (1,2), (1,2,3) ])
gap> Elements(last);
[ (), (2,3), (1,2), (1,2,3), (1,3,2), (1,3) ]
gap> MagmaWithInverses(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
Group([ (1,2), (1,2,3) ])
gap> Elements(last);
[ (), (2,3), (1,2), (1,2,3), (1,3,2), (1,3) ]

@codecov
Copy link

codecov bot commented Oct 23, 2017

Codecov Report

Merging #1798 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1798      +/-   ##
==========================================
+ Coverage    62.8%   62.92%   +0.12%     
==========================================
  Files         968      968              
  Lines      294774   294058     -716     
  Branches    13026    12963      -63     
==========================================
- Hits       185130   185043      -87     
+ Misses     106845   106216     -629     
  Partials     2799     2799
Impacted Files Coverage Δ
lib/magma.gi 49.9% <100%> (+1.57%) ⬆️
src/stats.c 75.47% <0%> (-0.14%) ⬇️
src/hpc/traverse.c 78.21% <0%> (-0.09%) ⬇️
src/compiler.c 61.92% <0%> (-0.05%) ⬇️
src/hpc/thread.c 46.44% <0%> (ø) ⬆️
src/objset.c 82.59% <0%> (ø) ⬆️
src/gap.c 56.83% <0%> (+0.08%) ⬆️
src/lists.c 56.71% <0%> (+0.11%) ⬆️
src/funcs.c 71.96% <0%> (+0.14%) ⬆️
... and 2 more

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

@gwhitney Hi there, thanks for your report, even including a fix! Very much appreciated.

I took the liberty of slightly editing your report, and in particular, of inlining the two transcripts.

Ideally, if you feel up to it, you could use your "corrected" transcript as basis for a proper test which ensures that the bug will not resurface later on, by adding a new .tst file under tst/testbugfix/ (look at that dir for some examples, and feel free to ask if you have any questions.

That would help us safe some effort (if you cannot or do not want to do it, one of us will have to do it, which means it will take longer to process this pull request).

@gwhitney
Copy link
Author

OK, hopefully that does it; let me know if there's any other way I can facilitate this patch. Best, Glen

Copy link
Contributor

@stevelinton stevelinton left a comment

Choose a reason for hiding this comment

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

This looks clearly correct to me.

@ChrisJefferson ChrisJefferson merged commit e5389a6 into gap-system:master Oct 26, 2017
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 22, 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.

5 participants