-
Notifications
You must be signed in to change notification settings - Fork 126
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
First interface for invariant theory of finite groups #584
Conversation
Co-authored-by: Tommy Hofmann <thofma@gmail.com>
@@ -0,0 +1,185 @@ | |||
|
|||
mutable struct InvRing{S, T, U, V, W, X} |
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 would be better if those one letter type names were replaced by more verbose (and self-explanatory) names, say
- S -> FldT
- T -> PolyRingT
- U -> GrpT
- V -> ActionT
- W -> SingularActionT
- X -> PolyElemT
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 would prefer that as well. @thofma, you added those, anything against this?
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.
Be my guest. I would have thought that the field names are self-explanatory, but we can make it more redundant.
While you are at it, you might want to change T
and U
so that the group type is the second parameter. This will make it a bit easier when adding methods for specific group types (i.e. InvRing{_, <: MatrixGroup}
vs InvRing(_, _, <: MatrixGroup)
.)
@@ -0,0 +1,185 @@ | |||
|
|||
mutable struct InvRing{S, T, U, V, W, X} |
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 think InvRing
should be turned into an actual ring type, so in particular subtype Ring
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 agree of course that one would expect InvRing
to be a Ring
. However, as you already pointed out, according to the documentation, rings have to comply to a lot of things and in particular need to have elements. I know that this discussion is already going on elsewhere (and in the end I implement whatever you want), but here are my two pennies on it: I would not expect a user to want to do computations in the invariant ring as a subring of the polynomial ring the group acts on. I would think that one wants to have a presentation of the invariant ring as an affine algebra for this. So I would aim for the following interface: InvRing
is just a cache and if one wants a Ring
, there should be a function presentation(::InvRing)
which returns the affine algebra (polynomial ring modulo ideal, I'm not sure we have a type for such a quotient ring yet?) and the map to the InvRing
(or rather the polynomial ring in there?).
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.
There are quotient rings, see the docu under Commutative Algebra, with more functionality for affine algebras (see again the docu). Indeed, in particular for geometric invariant theory applications, a function presentation(::InvRing)
is crucial. Also, as a first step towards such a presentation, we should have a function fundamental_invariants::InvRing)
.
Both functions have be to be developed essential from scratch (but see also the functionorbit_variety
in SINGULAR).
molien_singular::Singular.smatrix | ||
primary_singular::Singular.smatrix | ||
|
||
function InvRing(K::S, G::U, action::Vector{V}) where {S, U, V} |
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.
Perhaps check that K
is a field (e.g. by adding a type constraint)?
primary_singular::Singular.smatrix | ||
|
||
function InvRing(K::S, G::U, action::Vector{V}) where {S, U, V} | ||
n = degree(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.
What is G::U
supposed to be? You call degree
on it... so perhaps a permutation group? Or is degree also supposed to be the dimension of the natural module of a matrix group?
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.
So far we can only handle matrix groups because we pass everything to Singular. So, degree
is the rank of the matrices as defined in Groups/matrices/MatGrp.jl:490
.
I would think that this constructor should not restrict the type or maybe U <: AbstractAlgebra.Group
if absolutely necessary. The user is not supposed to call this anyway.
R = polynomial_ring(IR) | ||
p = Vector{elem_type(R)}() | ||
for i = 1:ncols(P) | ||
push!(p, R(P[1, i])) |
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.
Based on the codecov messages, none of the code in this file is tested, not even with a trivial input. It'd be nice to get some simple tests.
Sorry for not doing so earlier, but I just read DEVELOPMENT.md and realized that it might be a good idea to move the directory |
Here is a first interface to the invariant theory in the finvar library of Singular.