-
Notifications
You must be signed in to change notification settings - Fork 160
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
Omit non-characters from PermChars
results
#1867
Omit non-characters from PermChars
results
#1867
Conversation
Up to now, it might happen that the result of `PermCandidatesFaithful` contains virtual characters that are not characters, provided that one prescribes a list of rational irreducible constituents and that possible permutation characters exist which have negative scalar products with some rational irreducible characters that are not among the given constituents. These cases appear to be rare, and the smallest example which I know runs about 20 minutes. I regard the change as a bugfix, since the documentation states that the result consists of characters.
Codecov Report
@@ Coverage Diff @@
## master #1867 +/- ##
==========================================
+ Coverage 63.49% 63.73% +0.24%
==========================================
Files 952 946 -6
Lines 286804 284417 -2387
Branches 12722 12753 +31
==========================================
- Hits 182103 181271 -832
+ Misses 101899 100354 -1545
+ Partials 2802 2792 -10
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a test. Even if the cases where the bug shows up are too time-consuming, adding a test that makes sure the code still runs when the new optional argument is passed will make sure the code doesn't "rot". Actually, 20 minutes is not too bad for a test in tst/testextra.
Thanks for the hint about |
tst/testextra/ctblpope.tst
Outdated
gap> START_TEST( "ctblpope.tst" ); | ||
|
||
## | ||
gap> m:= CharacterTable( "M" );; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before calling CharacterTable( "M" );;
, please put the line
gap> LoadPackage("ctbllib", false);;
This will ensure that the Character Table Library is loaded for the test, otherwise make testandard
will fail when no packages are loaded with this being the first of a number of diffs:
########> Diff in /circa/scratch/gap-jenkins/workspace/GAP-pull-request-quickt\
est/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/standard/label/kovacs/GAP-pull-reque\
st-snapshot/tst/testextra/ctblpope.tst:13
# Input is:
m:= CharacterTable( "M" );;
# Expected output:
# But found:
Error, sorry, the GAP Character Table Library is not loaded,
call `LoadPackage( "CTblLib" )' if you want to use it
########
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. A better idea would be indeed to run the test only when IsPackageMarkedForLoading("ctbllib","");
returns true
. This will not enforce loading the package when GAP is started with -r -A
options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. along the lines
gap> if IsPackageMarkedForLoading("ctbllib","") then
... do calculations
... if Length(cand)<>1 then
> Print( "Error in calculation of ...\n" );
> fi; fi;
tst/testextra/ctblpope.tst
Outdated
gap> START_TEST( "ctblpope.tst" ); | ||
|
||
## | ||
gap> m:= CharacterTable( "M" );; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. along the lines
gap> if IsPackageMarkedForLoading("ctbllib","") then
... do calculations
... if Length(cand)<>1 then
> Print( "Error in calculation of ...\n" );
> fi; fi;
Thanks @ThomasBreuer. I've run the test in this PR and in current master branch, and was able to reproduce both passing and failing the test. |
Up to now, it might happen that the result of
PermCandidatesFaithful
(which is one of the variants that are called by
PermChars
)contains virtual characters that are not characters.
This happens if one prescribes a list of rational irreducible constituents
and if possible permutation characters exist (according to the criteria listed
in the documentation, except for the condition to be a character)
that have negative scalar products with some rational irreducible character
that is not among the given constituents.
These cases appear to be rare,
the smallest example which I know runs about 20 minutes.
I regard the change as a bugfix,
since the documentation states that the result consists of characters.