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

Two workarounds for old issues. #3704

Merged
merged 2 commits into from
Oct 16, 2019
Merged

Two workarounds for old issues. #3704

merged 2 commits into from
Oct 16, 2019

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented Oct 14, 2019 •
edited by olexandr-konovalov
Loading

as the default assertions completely distort the cost -- all time is wasted
on checking homomorphism properties again and again.

This resolves #3533

@alex-konovalov : the second commit added later resolves #3517

@hulpke hulpke added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Oct 14, 2019
@hulpke
FIX: Set assertions to 0 for isomorphism test tests
62d6e1f 
as the default assertions completely distort the cost -- all time is wasted
on checking homomorphism properties again and again.

This resolves gap-system#3533
@coveralls
Copy link

coveralls commented Oct 15, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.0004%) to 84.501% when pulling 68088a0 on hulpke:fixes into c36061d on gap-system:master.

@hulpke hulpke changed the title FIX: Set assertions to 0 for isomorphism test tests Two workarounds for old issues. Oct 15, 2019
@ChrisJefferson
Copy link
Contributor

Just as a sanity check, is replacing an Assert with a return fail going to mess anything up, or is it handled somewhere sensible?

@hulpke
Copy link
Contributor Author

hulpke commented Oct 16, 2019

@ChrisJefferson
The code already handles the rare situation that the function does not discover all classes, in this case the calculation is restarted . (In the long run this means probably a rewrite). So returning failwill be handled properly, even though this PR does not do so.
(The intention was to have a minimal workaround that has no chance of introducing other problems.)

@ChrisJefferson
Copy link
Contributor

Great, thanks

@DominikBernhardt DominikBernhardt added topic: library regression A bug that only occurs in the branch, not in a release labels Oct 16, 2019
@hulpke hulpke merged commit 5e331a1 into gap-system:master Oct 16, 2019
@olexandr-konovalov olexandr-konovalov mentioned this pull request Oct 16, 2019
Closed
@olexandr-konovalov
Copy link
Member

This should be backported to 4.11.

@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Oct 17, 2019
fingolfin pushed a commit that referenced this pull request Oct 18, 2019
@hulpke @fingolfin
Two workarounds for old issues. (#3704)
ed958f0 
* FIX: Set assertions to 0 for isomorphism test tests

as the default assertions completely distort the cost -- all time is wasted
on checking homomorphism properties again and again.

This resolves #3533

* WORKAROUND: Bad random choices in rare cases

This resolves the remaining issue in #3517
@fingolfin
Copy link
Member

Backported to stable-4.11 in ed958f0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisJefferson ChrisJefferson ChrisJefferson approved these changes

Assignees
No one assigned
Labels
backport-to-4.11-DONE regression A bug that only occurs in the branch, not in a release release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library
Projects
None yet
Milestone
GAP 4.11.0
6 participants
@hulpke @coveralls @ChrisJefferson @olexandr-konovalov @fingolfin @DominikBernhardt