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: Maximal subgroups will work also if no tomlib available #3501

Merged
merged 2 commits into from
Jun 16, 2019

Conversation

fingolfin
Copy link
Member

This extracts a commit from PR #3494 by @hulpke

Fixes #3498

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Jun 14, 2019
@fingolfin
Copy link
Member Author

Perhaps this also fixes #3496?

@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Jun 15, 2019

One manual example is affected: https://travis-ci.org/gap-system/gap/jobs/545707527. A fix is part of commit c4116b0. The fix seems to work - tested it on manual examples and teststandard, those do not timeout any more.

A problem from #3496

########> Diff in /Users/alexk/GITREPS/gap/tst/testbugfix/2019-04-09-Lattice.t\
st:3
# Input is:
StructureDescription(g);;t:=TableOfMarks(g);;
# Expected output:
# But found:
Error, List Element: <list>[11] must have an assigned value
########

when all packages are loaded still stays - that one should be fixed elsewhere in #3494.

@olexandr-konovalov
Copy link
Member

This fix will still break testextra, but at least will not cause timeout and may be fixed later:

########> Diff in /Users/alexk/GITREPS/gap/tst/testextra/grpperm.tst:266
# Input is:
p:=List(p,x->Image(IsomorphismPermGroup(x)));
# Expected output:
[ <permutation group with 13 generators>,
  <permutation group with 13 generators>,
  <permutation group with 13 generators>,
  <permutation group with 13 generators> ]
# But found:
[ <permutation group with 13 generators>, 
  <permutation group with 13 generators>, 
  <permutation group with 13 generators>, 
  <permutation group with 13 generators> ]
########

Look identical, eh? The diff is in a space after the comma. Cf. the idea of comparing output up to trailing whitespaces from #697.

@codecov
Copy link

codecov bot commented Jun 16, 2019

Codecov Report

Merging #3501 into master will decrease coverage by 0.06%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master    #3501      +/-   ##
==========================================
- Coverage   85.43%   85.36%   -0.07%     
==========================================
  Files         699      699              
  Lines      346760   346766       +6     
==========================================
- Hits       296253   296025     -228     
- Misses      50507    50741     +234
Impacted Files Coverage Δ
lib/twocohom.gd 100% <ø> (ø) ⬆️
lib/maxsub.gi 66.57% <0%> (-0.46%) ⬇️
lib/grplatt.gi 78.31% <100%> (ø) ⬆️
lib/process.gi 36% <0%> (-8%) ⬇️
lib/float.gd 96.77% <0%> (-3.23%) ⬇️
lib/info.gi 82.53% <0%> (-2.63%) ⬇️
lib/grpfp.gi 79.39% <0%> (-2.07%) ⬇️
lib/relation.gi 71.7% <0%> (-1.91%) ⬇️
lib/dict.gi 77.47% <0%> (-1.27%) ⬇️
lib/magma.gi 89.11% <0%> (-0.96%) ⬇️
... and 20 more

@coveralls
Copy link

coveralls commented Jun 16, 2019

Coverage Status

Coverage decreased (-0.002%) to 85.256% when pulling f9eeeb9 on fingolfin:mh/fix-fr into ff93de2 on gap-system:master.

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.

Note that -- as requested -- I now review what is de facto my own code.
It would be nice if #3494 also could be approved, as waiting for the formal approval takes resources.

@fingolfin
Copy link
Member Author

Are you sure that breakage in tst/testextra/grpperm.tst:266 is caused by this PR, and not rather by some other change, and only uncovered by this PR fixing the timeout in tst/testextra/grpperm.tst? Because I don't see how this PR could possible cause that diff.

Those spaces at the end of the lines really should be there, so it seems more likely to me that this was already broken when the test was added in commit 97711e4 / PR #3451. I've now fixed this via another commit in this PR.

@olexandr-konovalov
Copy link
Member

Thanks @fingolfin !

@olexandr-konovalov olexandr-konovalov merged commit ce16a7d into gap-system:master Jun 16, 2019
@olexandr-konovalov
Copy link
Member

And yes, I agree with your analysis @fingolfin (hence rebase and merge is OK for this PR). When the test run out of time, tst/testextra/grpperm.tst was never reached at all.

@fingolfin fingolfin deleted the mh/fix-fr branch June 16, 2019 21:28
@DominikBernhardt DominikBernhardt added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Aug 20, 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 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.

Three more failures in the master branch when GAP started with -r -A options: it hangs forever on
5 participants