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.

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

Put MatrixGroup into gap-system/gap or rename it? #62

Open
ssiccha opened this issue Oct 7, 2021 · 2 comments
Open

Put MatrixGroup into gap-system/gap or rename it? #62

ssiccha opened this issue Oct 7, 2021 · 2 comments

Comments

@ssiccha
Copy link
Collaborator

ssiccha commented Oct 7, 2021

@fingolfin Do you know of a technical reason why nobody created a MatrixGroup function until now? Or is it just that nobody bothered or saw the need for it until now? The main functionality for us currently is, that we want to be able to control DefaultFieldOfMatrixGroup, which we use e.g. in ConjugateToStandardForm. I guess that just never was important to anybody.

If that's more work than it's worth, we can simply rename MatrixGroup to something like ClassicalMaximals_MatrixGroup.

The same holds for MatrixGroupWithSize. Also, is there a technical reason why there is no GroupWithSize function? It's not like that's terribly important, but I find it makes a nice convenience function.

@fingolfin
Copy link
Member

Right now, MatrixGroup as implemented here is not very useful, so I think it's obvious why it's not in GAP, in this form. That said, there are GroupsWithGenerator methods which explicitly set the DefaultFieldOfMatrixGroup attribute. It would make sense to have a way to specify a GAP group with this attribute inside GAP.

So from that point of view, adding a MatrixGroup helper in GAP might make sense. Alternatively, one could extend Group to allow the first argument to be a ring (so Group(F, mat1, mat2, ...) or Group(F, [mat1, mat2, ...]). Likewise, for the implementation, one could also have GroupWithGenerators and GroupWithGenerators methods taking a field as first argument, OR add MatrixGroupByGenerators and MatrixGroupWithGenerators operations. Personally I'd be fine with either, but some other long time GAP developers might have additional thoughts why to prefer one over the other.

As to MatrixGroupWithSize and GroupWithSize: I don't like these at all; they are maybe OK for adhoc code, but otherwise they are super niche and basically save you tying a dozen characters, and leave you wondering why there isn't also GroupWithFinitiness (which takes a boolean as second argument indicating whether the group is finite or not) or alternatively FiniteGroup and InfiniteGroup; orSolvableGroup (which automatically sets that the group is solvable), etc.

So in this sense, I think they are examples of bad convenience functions: they save a few characters of typing but only support a subset of the features of Group+SetSize and add mental complexity (now you need to know yet another function).

@ssiccha
Copy link
Collaborator Author

ssiccha commented Oct 7, 2021

About GroupWithSize: not that I think we should implement what I'm writing now, but I guess then something like GroupWithAttributes, which would additionally take pairs of attributes and values, would be a more sensible convenience function. Again, not that I think we should actually do that.

About MatrixGroup: I've thought a little about it and want to open a PR for that, but didn't get to it yet. My idea would be to use MatrixObj matrices, because they can store their BaseDomain. At the moment Group already determines the base field of the given generators, if they're matrices, via DefaultScalarDomainOfMatrixList. Afterwards Group calls ImmutableMatrix. Both of these functions don't handle MatrixObj matrices well yet. DefaultScalarDomainOfMatrixList doesn't check their BaseDomain attribute, ImmutableMatrix doesn't even have a method installed for MatrixObj matrices we constructed over GF(7 ^ 4) and over the Gaussian integers. If we extend these, then controlling the DefaultFieldOfMatrixGroup for Group will work when using MatrixObj matrices.

Then for now, I'd rename MatrixGroup to something else, dunno a name yet, and just let it create MatrixObj matrices from the given generators and then call Group. In the long run, we'd have to create MatrixObj matrices from the start, so that for bigger fields, we don't create the same matrix twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants