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

Omit non-characters from PermChars results #1867

Merged

Conversation

ThomasBreuer
Copy link
Contributor

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.

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
Copy link

codecov bot commented Nov 7, 2017

Codecov Report

Merging #1867 into master will increase coverage by 0.24%.
The diff coverage is 87.5%.

@@            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
Impacted Files Coverage Δ
lib/ctblpope.gi 73.85% <87.5%> (+0.03%) ⬆️
lib/session.g 56% <0%> (-34.33%) ⬇️
hpcgap/pkg/gapdoc/lib/Text.gi 41.11% <0%> (-4.79%) ⬇️
hpcgap/pkg/gapdoc/lib/TextThemes.g 76.71% <0%> (-3.6%) ⬇️
lib/queue.g 66.4% <0%> (-3.2%) ⬇️
hpcgap/pkg/gapdoc/init.g 40% <0%> (-3.14%) ⬇️
src/weakptr.c 80.7% <0%> (-1.97%) ⬇️
lib/pcgsperm.gi 85.71% <0%> (-1.88%) ⬇️
hpcgap/lib/pcgsperm.gi 81.46% <0%> (-1.87%) ⬇️
hpcgap/lib/mapping.gi 78.34% <0%> (-0.63%) ⬇️
... and 122 more

Copy link
Contributor

@stevelinton stevelinton left a 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.

@ThomasBreuer
Copy link
Contributor Author

Thanks for the hint about tst/testextra, I have added a testfile.

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Nov 15, 2017
gap> START_TEST( "ctblpope.tst" );

##
gap> m:= CharacterTable( "M" );;
Copy link
Member

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
########

Copy link
Member

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.

Copy link
Member

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;

@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Nov 15, 2017
gap> START_TEST( "ctblpope.tst" );

##
gap> m:= CharacterTable( "M" );;
Copy link
Member

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;

@olexandr-konovalov olexandr-konovalov merged commit 4aea72e into gap-system:master Nov 16, 2017
@olexandr-konovalov
Copy link
Member

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.

@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 22, 2018
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.

4 participants