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

Workaround for error #3055 and fix for GQuotient #3110

Merged
merged 2 commits into from
Dec 18, 2018

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented Dec 14, 2018

This is a bug that arises very rarely, maybe based on random
selections. In this case restart calculations.

(The bug was reported in #3055)

Also fix for GQuotient running slow in certain cases of groups with infinite quotients.

@olexandr-konovalov
Copy link
Member

Thanks @hulpke! I've edited the description to add #3055 to the text - otherwise there is no visible link to click to navigate to it (one should either do it manually or click trough the commit message).

@codecov
Copy link

codecov bot commented Dec 17, 2018

Codecov Report

Merging #3110 into master will decrease coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #3110      +/-   ##
==========================================
- Coverage   83.57%   83.57%   -0.01%     
==========================================
  Files         686      685       -1     
  Lines      336779   336512     -267     
==========================================
- Hits       281461   281227     -234     
+ Misses      55318    55285      -33
Impacted Files Coverage Δ
lib/clashom.gi 78.87% <50%> (-0.07%) ⬇️
src/blister.h 88.28% <0%> (-0.91%) ⬇️
lib/init.g 80.03% <0%> (-0.55%) ⬇️
src/system.c 69.63% <0%> (-0.1%) ⬇️
src/gap.c 83.2% <0%> (-0.01%) ⬇️
src/hpc/thread.c 56.91% <0%> (ø) ⬆️
src/sysfiles.c 40.59% <0%> (ø) ⬆️
src/libgap-api.c 20.16% <0%> (ø) ⬆️
src/julia_gc.c
lib/streams.gi 70.45% <0%> (+0.21%) ⬆️
... and 2 more

This is a bug that arises very rarely, maybe based on random
selections. In this case restart calculations.
@hulpke hulpke changed the title Workaround for error #3055 Workaround for error #3055 and fix for GQuotient Dec 17, 2018
@hulpke hulpke added kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them topic: library labels Dec 17, 2018
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.

I'd approve the changes, but the test in its current form does not seem suitable for merging.

# index.
gap> F := FreeGroup("a", "b", "c");;
gap> G := F/ParseRelators(F,"a2b5a2b2C2,a5c3B2");;
gap> Length(GQuotients(G,AlternatingGroup(5)));
Copy link
Member

Choose a reason for hiding this comment

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

This tests was running for several minutes on my machine, then run out of memory.

That's not acceptable for tst/testbugfix, whch we run as part of our CI on every pull request. It could be added to tst/testextra, though (perhaps in a new dir tst/testextra/bugfix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check. This example should run in under a second. Probably it is the assertions again...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dear @fingolfin
That is really strange. I just rebased the branch, recompiled and ran the file with Test, and it took indeed under a second. Also, in the travis log I get

testing: /home/travis/build/gap-system/gap/tst/testbugfix/2018-12-17-gquotient.tst
 923 ms (24 ms GC) and 28.7MB allocated for 2018-12-17-gquotient.tst

which is the same magnitude.
Is it possible that you are loading something else that interferes? Could it be the Julia issue Thomas was observing?

Copy link
Member

Choose a reason for hiding this comment

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

I am not using Julia builds of GAP for my regular work, so no, that can't be it.

However, I cannot reproduce this anymore (with your branch). Not sure what went wrong...

@hulpke hulpke merged commit ddf633d into gap-system:master Dec 18, 2018
@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Mar 21, 2019
@PaulaHaehndel PaulaHaehndel added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 15, 2019
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants