Fix FroidurePinExtendedAlg for partial perm monoids #1697
Conversation
lib/semirel.gi
Outdated
| @@ -972,13 +972,14 @@ function(m) | |||
| fi; | |||
|
|
|||
| #gens:=Set(GeneratorsOfMonoid(semi)); | |||
| gens:=Set(Filtered(GeneratorsOfMonoid(semi), x-> not IsOne(x))); | |||
| one := One(semi); | |||
There was a problem hiding this comment.
I'd prefer a uniform formatting here: either use one:=One(semi); here or put the spaces on all the other assignments.
wilfwilson
force-pushed
the
fix-issue-1674
branch
from
8f7e1ac
to
a269a0c
Compare
Codecov Report
@@ Coverage Diff @@
## master #1697 +/- ##
==========================================
- Coverage 64.42% 64.42% -0.01%
==========================================
Files 1002 1002
Lines 326711 326713 +2
Branches 13218 13218
==========================================
- Hits 210489 210477 -12
- Misses 113357 113361 +4
- Partials 2865 2875 +10
|
olexandr-konovalov
added
the
gapdays2017-fall
|
This looks good to me, maybe @james-d-mitchell can have a look (and merge if he's happy). |
| @@ -972,13 +972,14 @@ function(m) | |||
| fi; | |||
|
|
|||
| #gens:=Set(GeneratorsOfMonoid(semi)); | |||
| gens:=Set(Filtered(GeneratorsOfMonoid(semi), x-> not IsOne(x))); | |||
| one:=One(semi); | |||
| gens:=Set(Filtered(GeneratorsOfMonoid(semi), x -> x <> one)); | |||
There was a problem hiding this comment.
So, really this is the only changed line here, right? I'm not sure it's worth changing the other lines too (i.e. by replacing One(semi) by one).
There was a problem hiding this comment.
While I don't know what Wilf's motivation was, I think it's quite natural to use the already computed value of one instead of recomputing it a few times later on -- this is a (albeit fairly minor, but still natural) optimization.
There was a problem hiding this comment.
This ship has sailed but just to say that One is an attribute of a semigroup and so only calculated on the first call...
|
@markuspf @wilfwilson looks good to me, apart from I don't see why |
olexandr-konovalov
added
the
release notes: added
In #1674 I gave an example of a monoid of partial perms, on which
GreensDClassesgave an incorrect result. This PR resolves #1674.@markuspf tracked the cause down to a line in
FroidurePinExtendedAlg. The problem is that a line in this method assumed that theOneof any element of a monoid is the same as theOneof the monoid. This is the documented behaviour of howOneworks in a magma-with-one, but this is not how monoids of partial perms behave. I will open an issue in the hope of addressing the disparity between behaviour and documentation.