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

Permutations to get alignment of ordering with Magma #39

Closed
wants to merge 4 commits into from

Conversation

hulpke
Copy link
Contributor

@hulpke hulpke commented Oct 10, 2018

These have been explicitly checked by comparing CodePcGroup.

Program used: In Magma, fetch data through

p:=3;e:=7;
l:=SearchPGroups(p,e);;
SetLogFile("gnum3e7.g");
print "l:=[\n";
for i in [1..#l] do
  a,b:=SmallGroupEncoding(l[i]);
  print a,",";
end for;
print "];";

(Then edit the file to throw out junk) and compare in GAP.

The permutations applied had been wrongly implemented and were based on
wrong data. New permutations, based on explicitly comparing with Magma data
were used to deermine correct permutations.

The test for the permutations in `ordering.tst` was changed, as the old test
relied on the permutations having small support, which is not true any
longer.

This will fix gap-packages#38
as they required that the permutations were of small support. (They also were
wrong)
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.

Very good, thank you very much. This should ideally be released ASAP, so that it is ready for GAP 4.10.

It would be great if there also was a CHANGES or NEWS file in the release, which points out this change (and any other that may have happened since 1.3).

CC @alex-konovalov

@@ -1,34 +1,7 @@
gap> START_TEST("ordering.tst");;

Copy link
Member

Choose a reason for hiding this comment

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

You removed a comment sign, and now tests fail - please restore it or remove an empty line at all, otherwise tests fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Will be fixed.

@codecov
Copy link

codecov bot commented Oct 10, 2018

Codecov Report

Merging #39 into master will increase coverage by 11.03%.
The diff coverage is 98.31%.

@@             Coverage Diff             @@
##           master      #39       +/-   ##
===========================================
+ Coverage   88.75%   99.79%   +11.03%     
===========================================
  Files          22      491      +469     
  Lines        4589   325067   +320478     
===========================================
+ Hits         4073   324395   +320322     
- Misses        516      672      +156
Impacted Files Coverage Δ
gap/small.gd 100% <100%> (ø) ⬆️
gap/small.gi 95.74% <33.33%> (-1.23%) ⬇️
small11/smlgp11.g 13.29% <0%> (-81.73%) ⬇️
small5/smlgp5.g 100% <0%> (ø) ⬆️
gap/idgrp1.g 100% <0%> (ø) ⬆️
gap/smlgp1.g 100% <0%> (ø) ⬆️
id5/idgrp5.g 100% <0%> (ø) ⬆️
id2/id896.zan 100% <0%> (ø)
id2/id256.l 100% <0%> (ø)
... and 483 more

@hulpke
Copy link
Contributor Author

hulpke commented Oct 10, 2018

@fingolfin
Yes, there needs to be more documentation and Bettina was planning to write explicitly to the gap-forum. I just created the branch now there is no duplication of work, as I saw @markuspf had started as well.

@hulpke
Copy link
Contributor Author

hulpke commented Oct 10, 2018

Please do not merge yet -- I'm still in discussion with Bettina and Eamonn on the correct approach

@hulpke
Copy link
Contributor Author

hulpke commented Oct 11, 2018

@alex-konovalov Thanks for creating the do not merge label!

@markuspf
Copy link
Member

@hulpke I had started, but then abandoned doing stuff, because I noticed that the fix would be much more complicated than what I first thought (and I saw you doing the more complicated fix already).

@markuspf
Copy link
Member

As an additional note:

I would quite like to know how the comparison between MAGMA and GAP happened; Do we have code to re-run this/validate this?

If we want to insist that the numberings between GAP and MAGMA are consistent, it would be good if we had a way to verify this, at least in terms of some random samples.

I tried but couldn't really be bothered finding out how to even compare groups between MAGMA and GAP (that is probably entirely my own laziness/inability), because I couldn't even easily figure out how to get a pcp out of MAGMA.

@olexandr-konovalov
Copy link
Member

Would be nice to have some progress and make a release in October to ensure it goes into GAP 4.10.

@hulpke
Copy link
Contributor Author

hulpke commented Oct 23, 2018

@alex-konovalov Do not delay 4.10 for catching this update. The change will be larger as it will also have to involve Magma, there will be an update of the package in due course, but not synchronized with the GAP release.

@hulpke
Copy link
Contributor Author

hulpke commented Oct 23, 2018

@markuspf The comparison was done by printing out the encoding (equivalent to CodePcGroup in GAP), as listed on the very top of this PR.

@olexandr-konovalov
Copy link
Member

@hulpke do you have any updates, by any chance? This PR still has "do not merge" label, and the branch has now conflicts with master...

@hulpke
Copy link
Contributor Author

hulpke commented Feb 19, 2019

No update from me. In fact the problem will likely be dealt with by explicitly not promising an order for p^7 (as part of it might be generated on the fly in Magma and thus could change order).

I will thus close this PR.

@hulpke hulpke closed this Feb 19, 2019
@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented Sep 14, 2019

A new attempt is now suggested in #46 - thanks @hulpke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants