-
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
Improvements to reesmat code and new tests #1676
Conversation
Previously, IsFinite used the logic that for a subsemigroup <S> of a Rees (0-)matrix semigroup <R>, <S> is finite if and only if <R> is finite. You can use the information that <R> is finite to deduce that <S> is finite, but if <R> is infinite you cannot immediately deduce the (in)finiteness of <S>. Resolves gap-system#1659
IsOne: IsOne for a Rees (0-)matrix semigroup element doesn't make sense, since a Rees (0-)matrix semigroup is never IsMagmaWithOne. Furthermore, even interpreting `IsOne` as menaing `IsMultiplicativeNeutralElement`, the methods were invalid. Therefore, I've removed them. Resolves gap-system#1661
05be26d
to
88028b8
Compare
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.
Looks very clean and orderly to me, only have some very small and optional remarks.
However, I don't know anything about the mathematical side of this, so it'd be great if somebody who does could glance at it as well.
@@ -77,15 +77,21 @@ end); | |||
InstallMethod(IsIdempotent, "for a Rees 0-matrix semigroup element", | |||
[IsReesZeroMatrixSemigroupElement], | |||
function(x) | |||
local R; | |||
local matrix_entry, g; |
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.
commit message: "no affect on performance" should be "no effect on performance"
lib/reesmat.gi
Outdated
fi; | ||
|
||
g := UnderlyingElementOfReesZeroMatrixSemigroupElement(x); | ||
return g * matrix_entry * g = g; |
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.
Since I don't know how these things are stored internally, I cannot at all determine whether this change is valid. (E.g. an inversion happened in the old code which didn't happen before).
So I'll just trust that you know what you are doing :-).
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.
In case you are interested @fingolfin - if not, no need to read further!:
A Rees matrix semigroup element is a triple (i, g, j)
, where i
and j
are indices (referring to a column and row of a matrix, respectively), g
is an element of a semigroup, and the multiplication is
(i, g, j)(k, h, l) = (i, g * mat[j][k] * h, l)
where mat
is the matrix associated to the Rees matrix semigroup. So an element is an idempotent whenever g * mat[j][i] * g = g
. (The situation is slightly more complicated for Rees 0-matrix semigroups, but never mind).
For the special case that g
is an element of a group (rather than of just a semigroup, in general), you can rearrange that equation to be g = mat[j][i] ^ -1
; this is the condition that the old code checked. A better check would've been g * mat[j][i] = One(g)
, to avoid inversion.
The version I went with works in general (not just for groups), and doesn't seem to noticeably slow things down.
lib/reesmat.gi
Outdated
|
||
if ForAny(Columns(R), j -> ForAll(Rows(R), i -> Matrix(R)[j][i] = 0)) | ||
or ForAny(Rows(R), i -> ForAll(Columns(R), j -> Matrix(R)[j][i] = 0)) then | ||
return false; |
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.
Minor optimization: perhaps store Matrix(R)
into a local variable.
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'll make this change
I see that @markuspf also reviewed this, so I guess we can merge this once (or "if" ???) Travis finishes with it. |
This is a very minor change which (in my opinion) makes the code more readable, and seems to have no effect on performance.
This is a very minor change, which means that the good method in `IsRegularSemigroup` applies to a slightly larger class of argument.
This commit fixes a bug that could lead to a break loop, and also replaces the statement `return Size(ParentAttr(R)) = 1;` with `return false`, since a Rees 0-matrix semigroup is never trivial.
This commit shortens a method for `IsZeroSimpleSemigroup`, so that it uses `ForAll` and `ForAny` instead of loops; I think this is more readable.
In IsReesMatrixSemigroup, this commit removes some code that I believe is unreachable for finite semigroups, and invalid for infinite semigroups. It had no test coverage previously.
This commit fixes a bug whereby an Enumerator of a Rees (0-)matrix semigroup believed that it contains elements from a different Rees (0-)matrix semigroup.
My tests haven't even started running yet - so I may as well fix the minor things pointed out by @fingolfin and push them. |
88028b8
to
a914d63
Compare
Done that. Thanks for reviewing. Hopefully Travis will have started (and maybe even finished) by tomorrow. |
Codecov Report
@@ Coverage Diff @@
## master #1676 +/- ##
==========================================
+ Coverage 64.35% 64.42% +0.06%
==========================================
Files 1002 1002
Lines 326699 326705 +6
Branches 13218 13218
==========================================
+ Hits 210244 210475 +231
+ Misses 113585 113361 -224
+ Partials 2870 2869 -1
|
This PR resolves issues #1659 and #1661, as well as a couple of other small bugs I detected and fixed whilst working on this PR.
My purpose with this PR was to increase my confidence in the library's Rees (0-)matrix semigroup code, since I'd never really looked at it before. I was concerned with making sure that the code correct rather than more efficient, and I was writing test coverage as a way of achieving this confidence.
@james-d-mitchell already had good coverage of
lib/reesmat.gi
with his test filetst/teststandard/reesmat.tst
, which contains more complicated tests than the ones I have added in this PR. I have put my new tests intotst/testinstall/reesmat.tst
, which on its own gives 98% coverage oflib/reesmat.gi
. I started my new file so that I could approach the problem fresh.In doing so, I found several instances where methods either returned wrong answers, or unnecessarily led to break loops. I have fixed these problems, and re-written some very small parts of the code for either improved readability or consistency. More detail on the specific changes is given in the relevant commit messages. In summary, there were problems with
IsFinite
IsOne
Enumerator
IsReesMatrixSemigroup
IsReesZeroMatrixSemigroup
This PR increases the code coverage of
lib/reesmat.gi
almost as far as I can without running into technical difficulties (having test coverage of some errors, for instance, will mean that the tests will fail when the Semigroups package is loaded, since Semigroups replaces some of the relevant methods, and uses different error messages). I make no claim thatlib/reesmat.gi
is perfect now, but I think this PR is a good start.I apologise for this being a large number of changes in one PR; however most of the lines are simply new tests. The changes to
lib/reesmat.gi
themselves are quite small and I think it makes sense to bundle them together.