-
Notifications
You must be signed in to change notification settings - Fork 161
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
Fix GreensXClasses for a Green's class #2830
Fix GreensXClasses for a Green's class #2830
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2830 +/- ##
==========================================
+ Coverage 76.05% 76.05% +<.01%
==========================================
Files 480 480
Lines 241022 241022
==========================================
+ Hits 183310 183311 +1
+ Misses 57712 57711 -1
|
lib/semirel.gd
Outdated
## <Attr Name="GreensLClasses" Arg='S'/> | ||
## <Attr Name="GreensHClasses" Arg='S'/> | ||
## <Attr Name="GreensJClasses" Arg='S'/> | ||
## <Attr Name="GreensDClasses" Arg='S'/> |
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.
I'm not sure the rationale behind using " and ' here?
lib/semirel.gi
Outdated
@@ -720,13 +720,13 @@ function(semi) | |||
end); | |||
|
|||
InstallOtherMethod(GreensHClasses, "for a Green's Class", true, [IsGreensDClass], 0, |
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 might be better to DeclareAttribute("GreensHClasses", IsGreensDClass);
and drop the Other
here, but maybe there is some reason I don't know for doing it this way.
Also I believe the true
and the 0
are redundant, and could be removed.
Previously, there was undocumented and broken support for computing the GreensXClasses of a Green's class. I have now fixed and documented this code.
7285f12
to
8d04adf
Compare
Thank you for reviewing @james-d-mitchell |
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.
Seems fine, James is happy -- please merge if you are happy with it as-is
@fingolfin: I am happy with it as-is. @james-d-mitchell revoked his write access to GAP, and I never had write access, so it'll have to be merged by someone else. |
Previously, there was undocumented and broken support for computing the
GreensXClasses
(whereX
is in{H, L, R}
contained in a particular type of Green's class. This arose from the following issue on the Semigroups package issue tracker: semigroups/Semigroups#413I have now made a simple fix, and documented this behaviour (it doesn't have brilliant performance, but it is better than an error).