correction of IsMonomial, mainly for reducible characters
#2113
Conversation
There was a problem hiding this comment.
Thanks, this looks quite good to me. I just have some very minor comments and questions.
Also, could you please edit the commit message to start with a single short summary (ideally with <= 60 characters), followed by an empty line, followed by a longer description (e.g. the list you have now)? That is the recommend format for git commit messages; git, GitHub and many other tools display the first line of a commit in many contexts, so having a meaningful short summary in it is very helpful
| @@ -357,6 +364,10 @@ end ); | |||
| ## | |||
| #M TestQuasiPrimitive( <chi> ) . . . . . . . . . . . . . . . for a character | |||
| ## | |||
| ## This works also for reducible <chi>. | |||
| ## Note that a representation affording <chi> maps the centre of <chi> | |||
| ## to scalar matrices. | |||
There was a problem hiding this comment.
What is the relevance of this comment (on the centre) at this point? To me, a comment here is for somebody who wants to call the function without caring what it does... Wouldn't this factoid about the centre be more helpful below, perhaps near the "proof" comment?
There was a problem hiding this comment.
No.
Somebody who wants to call the function should look at the documentation
in the corresponding .gd file.
Comments in the implementation part are intended for somebody who wants to look at the code.
lib/ctblmono.gi
Outdated
| @@ -1013,6 +1015,8 @@ InstallMethod( IsMonomialNumber, | |||
| ## | |||
| #M TestMonomialQuick( <chi> ) . . . . . . . . . . . . . . . for a character | |||
| ## | |||
| ## We may assume that <chi> is an irreducible character. | |||
There was a problem hiding this comment.
"We may assume"? This makes it sound as if it was optional, or as if there was some underlying reason... Remove "may"?, resp. say "Assumes that..." ?
Or even clarify: "This function assumes that is an irreducible character, but does not verify this. Other inputs may thus leading to nonsensical output." ?
lib/ctblmono.gi
Outdated
| # Start with elementary tests for monomiality. | ||
| if IsIrreducibleCharacter( chi ) then | ||
| test:= TestMonomialQuick( chi ); | ||
| elif HasIsMonomialCharacter( chi ) then |
There was a problem hiding this comment.
Should that HasIsMonomialCharacter perhaps be first, given that it'll be the quickest test by far?
|
Something went wrong there -- you now have two commits with identical (?) content and a merge commit in this PR. Perhaps try to |
ThomasBreuer
force-pushed
the
TB_IsMonomial
branch
from
3521271
to
8c11d0f
Compare
|
@ThomasBreuer Unfortunately, this PR causes multiple test failures, already in |
ThomasBreuer
force-pushed
the
TB_IsMonomial
branch
from
8c11d0f
to
ca2494b
Compare
|
The tests pass now, but this PR contains duplicates of all kinds of commits on master. You'll have to |
fingolfin
added
kind: bug
- change the 'TestMonomialQuick' and 'TestMonomial' methods
for characters such that also reducible characters are handled correctly
- restrict the 'IsPrimitiveCharacter' method that relies on
quasi-primitivity to irreducible characters
- define 'IsSubnormallyMonomial' only for irreducible characters
- added tests
TB
ThomasBreuer
force-pushed
the
TB_IsMonomial
branch
from
ca2494b
to
9e94517
Compare
Codecov Report
@@ Coverage Diff @@
## master #2113 +/- ##
==========================================
+ Coverage 69.58% 69.67% +0.09%
==========================================
Files 482 482
Lines 254628 254639 +11
==========================================
+ Hits 177182 177432 +250
+ Misses 77446 77207 -239
|
fingolfin
added
the
release notes: added
correction of
IsMonomial, mainly for reducible characterschange the
TestMonomialQuickandTestMonomialmethodsfor characters such that also reducible characters are handled correctly
restrict the
IsPrimitiveCharactermethod that relies onquasi-primitivity to irreducible characters
define
IsSubnormallyMonomialonly for irreducible charactersadd tests