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.

Sign up for GitHub

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

Jump to bottom

Fix corner case in modified Todd-Coxeter when relator is trivial #3311

Merged
merged 1 commit into from
Feb 21, 2019
Merged

Fix corner case in modified Todd-Coxeter when relator is trivial #3311

merged 1 commit into from
Feb 21, 2019

Conversation

markuspf
Copy link
Member

No description provided.

@markuspf markuspf added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes backport-to-4.10 labels Feb 19, 2019
@markuspf
Copy link
Member Author

Arguably we could also filter out trivial relators when creating a finitely presented group, but this change makes the code safer regardless.

@markuspf markuspf requested review from hulpke and fingolfin and removed request for hulpke February 19, 2019 11:24
markuspf
markuspf commented Feb 19, 2019
View reviewed changes
lib/sgpres.gi Outdated
SortBy(rels,Length);
ri:=Union(rels,List(rels,x->x^-1));
ri:=List(ri,LetterRepAssocWord);
SortBy(ri,Length);
A:=Concatenation([1..m],-[1..m]);

Copy link
Member Author

Choose a reason for hiding this comment

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

I==

Suggested change

Copy link
Member

Choose a reason for hiding this comment

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

?

hulpke
hulpke approved these changes Feb 19, 2019
View reviewed changes
Copy link
Contributor

@hulpke hulpke left a comment

Choose a reason for hiding this comment

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

Yes, this will remove this problem. (It is good to do this in the TC only, but keep the trivial relator in the group.)

fingolfin
fingolfin requested changes Feb 20, 2019
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Please change the commit message to something more self-explanatory, something that does not require access to an unspecified external entity in order to make sense. E.g.
"Fix bug in coset enumeration for fp groups with trivial relator"

Other than this, this PR of course seems fine, thanks

lib/sgpres.gi Outdated
SortBy(rels,Length);
ri:=Union(rels,List(rels,x->x^-1));
ri:=List(ri,LetterRepAssocWord);
SortBy(ri,Length);
A:=Concatenation([1..m],-[1..m]);

Copy link
Member

Choose a reason for hiding this comment

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

?

@codecov
Copy link

codecov bot commented Feb 20, 2019 •
edited
Loading

Codecov Report

Merging #3311 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3311      +/-   ##
==========================================
- Coverage   85.23%   85.23%   -0.01%     
==========================================
  Files         696      695       -1     
  Lines      344083   343780     -303     
==========================================
- Hits       293278   293013     -265     
+ Misses      50805    50767      -38
Impacted Files Coverage Δ
lib/sgpres.gi 79.35% <100%> (ø) ⬆️
lib/init.g 82.93% <0%> (-0.37%) ⬇️
src/gap.c 81.04% <0%> (-0.12%) ⬇️
src/system.c 73.73% <0%> (-0.09%) ⬇️
src/julia_gc.c
src/objset.c 85.35% <0%> (+0.22%) ⬆️
src/weakptr.c 99.09% <0%> (+1.18%) ⬆️

@coveralls
Copy link

coveralls commented Feb 20, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.003%) to 85.074% when pulling ce0ad33 on markuspf:fix-cornercase into e7a9ec6 on gap-system:master.

fingolfin
fingolfin approved these changes Feb 20, 2019
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Perfect now, thank you :)

@fingolfin fingolfin changed the title Fixes #3310 Fix corner case in modified Todd-Coxeter when relator is trivial Feb 20, 2019
@fingolfin fingolfin merged commit d1666c7 into gap-system:master Feb 21, 2019
@fingolfin fingolfin mentioned this pull request Feb 21, 2019
Closed
@fingolfin fingolfin mentioned this pull request Jun 4, 2019
Merged
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.2 milestone Jun 12, 2019
@fingolfin fingolfin modified the milestone: GAP 4.10.2 Jun 13, 2019
@olexandr-konovalov olexandr-konovalov removed the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Jun 15, 2019
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jun 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

@hulpke hulpke hulpke approved these changes

Assignees
No one assigned
Labels
backport-to-4.10-DONE kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them 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
Milestone
GAP 4.10.2
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@markuspf @coveralls @fingolfin @hulpke @olexandr-konovalov