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.

Sign up for GitHub

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

Jump to bottom

addressing issue #2646: Unused functions in ctbl.gi #2681

Merged
merged 1 commit into from
Aug 14, 2018
Merged

addressing issue #2646: Unused functions in ctbl.gi #2681

merged 1 commit into from
Aug 14, 2018

Conversation

ThomasBreuer
Copy link
Contributor

  • changed the function CharacterTableOfNormalSubgroup such that either
    a full character table or fail is returned,
    and documented this function,

  • documented ClassPositionsOfSolvableRadical,

  • removed the unused and undocumented function
    CharacterTable_IsNilpotentNormalSubgroup
    (use IsSubset and ClassPositionsOfFittingSubgroup if you need
    what this function computes),

  • fixed an error when calling Display with the character table
    of a trivial group, and added a test example for that.

@fingolfin
Copy link
Member

This fails due to diffs in manual examples. From the travis log:

+bin/gap.sh --quitonbreak -b -L testmanuals.wsp --cover coverage/ref-chapter071.tst.coverage
########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/ctbl.gd:
4415 (/home/travis/build/gap-system/gap/tst/testmanuals/ref-chapter071.tst:
909)
# Input is:
kernel:= KernelOfCharacter( irr[3] );
# Expected output:
Group([ (1,2)(3,4), (1,4)(2,3) ])
# But found:
Group([ (1,2)(3,4), (1,3)(2,4) ])
########
########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/ctbl.gd:
4415 (/home/travis/build/gap-system/gap/tst/testmanuals/ref-chapter071.tst:
913)
# Input is:
NormalSubgroupClassesInfo( tbl );
# Expected output:
rec( nsg := [ Group([ (1,2)(3,4), (1,4)(2,3) ]) ],
  nsgclasses := [ [ 1, 3 ] ], nsgfactors := [  ] )
# But found:
rec( nsg := [ Group([ (1,2)(3,4), (1,3)(2,4) ]) ], 
  nsgclasses := [ [ 1, 3 ] ], nsgfactors := [  ] )
########
ref-chapter071.tst
msecs: 75376

fingolfin
fingolfin reviewed Aug 6, 2018
View reviewed changes
lib/ctbl.gi
#T !!
result:= Concatenation( "Rest(", Identifier( tbl ), ",",
String( classes ), ")" );
ConvertToStringRep( result );
Copy link
Member

Choose a reason for hiding this comment

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

I don't think calling ConvertToStringRep here is needed (but I see that this was just copied from the existing code, so it's OK to leave it in)

fingolfin
fingolfin reviewed Aug 6, 2018
View reviewed changes
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.

This looks good to me, except for the failing tests (and three minor, optional remarks)

lib/ctbl.gi
@@ -4991,9 +4958,11 @@ BindGlobal( "CharacterTableDisplayDefault", function( tbl, options )
elif centralizers = true then
Print( "\n" );
for i in [col..col+acol-1] do
fak:= Factors(Integers, tbl_centralizers[classes[i]] );
fak:= Factors( Integers, tbl_centralizers[ classes[i] ] );
Copy link
Member

Choose a reason for hiding this comment

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

Optional: This could also be changed to fak := PrimeDivisors( tbl_centralizers[ classes[i] ] );, then the call to Set in the next line and the check for prime <> 1 later would not be needed.

lib/ctbl.gd
## gap> g:= SchurCoverOfSymmetricGroup( 5, 3, 1 );;
## gap> c:= CyclicGroup( 4 );;
## gap> dp:= DirectProduct( g, c );;
## gap> diag:= First( Elements( Centre( dp ) ),
Copy link
Member

Choose a reason for hiding this comment

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

Optional: Using Elements here is not needed, it's perfectly fine to do First( Centre( dp ), ...)

@codecov
Copy link

codecov bot commented Aug 9, 2018 •
edited
Loading

Codecov Report

Merging #2681 into master will increase coverage by 0.04%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master    #2681      +/-   ##
==========================================
+ Coverage   75.43%   75.47%   +0.04%     
==========================================
  Files         478      478              
  Lines      241610   241604       -6     
==========================================
+ Hits       182259   182352      +93     
+ Misses      59351    59252      -99
Impacted Files Coverage Δ
lib/ctbl.gd 75.86% <ø> (ø) ⬆️
lib/ctbl.gi 74.07% <97.22%> (+2.15%) ⬆️
src/objset.c 84.82% <0%> (+0.22%) ⬆️
src/vec8bit.c 83.7% <0%> (+0.24%) ⬆️
lib/grpnice.gi 82.02% <0%> (+0.37%) ⬆️
lib/grp.gi 85.43% <0%> (+0.45%) ⬆️
lib/ctblauto.gi 83.17% <0%> (+0.76%) ⬆️
hpcgap/lib/hpc/stdtasks.g 64.19% <0%> (+0.76%) ⬆️

@fingolfin fingolfin force-pushed the TB_unused_in_ctbl.gi branch 2 times, most recently from f121b90 to df2a247 Compare August 10, 2018 13:41
fingolfin
fingolfin approved these changes Aug 10, 2018
View reviewed changes
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.

I took the liberty to address the diff in the manual examples, so the tests now seem to pass.

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: documentation Issues and PRs related to documentation topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 11, 2018
@ThomasBreuer @fingolfin
addressing issue gap-system#2646: Unused functions in ctbl.gi
a5e9c2f 
- changed the function `CharacterTableOfNormalSubgroup` such that either
  a full character table or `fail` is returned,
  and documented this function,

- documented `ClassPositionsOfSolvableRadical`,

- removed the unused and undocumented function
  `CharacterTable_IsNilpotentNormalSubgroup`
  (use `IsSubset` and `ClassPositionsOfFittingSubgroup` if you need
  what this function computes),

- fixed an error when calling `Display` with the character table
  of a trivial group, and added a test example for that.
@fingolfin fingolfin force-pushed the TB_unused_in_ctbl.gi branch from df2a247 to a5e9c2f Compare August 11, 2018 20:15
@fingolfin fingolfin merged commit 2a96670 into gap-system:master Aug 14, 2018
@olexandr-konovalov olexandr-konovalov mentioned this pull request Aug 16, 2018
Closed
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Sep 23, 2018
@ThomasBreuer ThomasBreuer deleted the TB_unused_in_ctbl.gi branch March 22, 2019 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
kind: bug Issues describing general bugs, and PRs fixing them kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: documentation Issues and PRs related to documentation topic: library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ThomasBreuer @fingolfin