Added more specialised function for testing conjugacy of subgroups of Sn #3231
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hulpke
approved these changes
Jan 25, 2019
Moved tst file to right location Added comments in testfile Changed comments Changed result for PerfectResiduum
Codecov Report
@@ Coverage Diff @@
## master #3231 +/- ##
==========================================
+ Coverage 84.75% 84.76% +0.01%
==========================================
Files 689 689
Lines 336391 336400 +9
==========================================
+ Hits 285107 285158 +51
+ Misses 51284 51242 -42
|
|
Looks good. I'll just let all the tests cycle. |
markuspf
approved these changes
Jan 25, 2019
markuspf
added
topic: performance
fingolfin
reviewed
Jan 26, 2019
| @@ -0,0 +1,14 @@ | |||
| # The following two test use the new version for natural symmetric group | |||
There was a problem hiding this comment.
"the new version" of what/? Just looking at this file doesn't make that clear. How about something more like this:
This file tests an
IsConjugatemethod for natural symmetric groups, and its various cases.
fingolfin
reviewed
Jan 26, 2019
| gap> IsConjugate( SymmetricGroup(200),PrimitiveGroup(200,4), PrimitiveGroup(200,3)); | ||
| false | ||
|
|
||
| # Here, using SubgpConjSymmgp yields a significant speedup |
|
Thanks, this is an improvement. However, the test file comments could have been better... :) When writing comments, never forget that a person reading a comment will usually not have the commit which added them as context, nor do they know anything about the motivation for writing the code in the first place. |
DominikBernhardt
added
release notes: added
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Directly using
SubgpConjSymmgpas a specialized method when testing two subgroups of a natural symmetric group for conjugacy in the full symmetric group often yields a speedup as for example in the last case in the test file.