New PR for action of MatrixGroup on a MPolynomialRing

[Bruno-TT]

fixes #4513

new PR to replace #35171 cause github is throwing server errors when I try to fix the merge conflict

I've replaced mRingPoly.variables() with mRingPoly.parent().gens() to get the correct behaviour, added some more comprehensive tests, used a better variable naming convention, and replaced the type(...)==... with an isInstance() call.

the code is still in .__call__ but I agree with @tscrim that we should probably move it somewhere else to make this action work in the coercion framework

[tscrim]

Let's see if we can figure out a better way to do this. I think the @ operator is the best way to do this

from sage.categories.action import Action
def MatrixPolynomialAction(Action):
    def __init__(self, MS, PR):
        self._poly_vars = PR.gens()
        self._vars_vector = matrix(self._poly_vars).transpose()
        super().__init__(MS, PR, op=operator.matmul)

    def _act_(self, mat, poly):
        vars_to_sub = matrix(poly.base_ring(), mat) * self._vars_vector
        substitution_dict = {v: s for v, s in zip(self._poly_vars, vars_to_sub)}
        return polynomial.subs(substitution_dict)

Then for the MPolynomialRing, have it implement

def _get_action_(self, G):
    if isinstance(G, (MatrixSpace, MatrixGroup)) and self.base_ring().has_coerce_map_from(G.base_ring()):
        return MatrixPolynomialAction(G, self)
    return super()

I haven't tested this, but if this doesn't work, something close to it should I think.

---

Some other comments if you don't want to go the above route.

Don't fix the linter issues in real_*; there is already an accepted PR for that.

Can you also address PEP8 spacing (here, a lack thereof) and remove the extra blanklines around the """?

[Bruno-TT]

Hi @tscrim I've made a few tweaks to get your suggestions compiling, but it's throwing an error

sage: G=groups.matrix.Sp(4,GF(2))
sage: R.<w,x,y,z>=GF(2)[]
sage: p=x*y^2 + w*x*y*z + 4*w^2*z+2*y*w^2
sage: g=G.1
sage: g
[0 0 1 0]
[1 0 0 0]
[0 0 0 1]
[0 1 0 0]
sage: g@p

...

TypeError: unsupported operand parent(s) for @: 'Symplectic Group of degree 4 over Finite Field of size 2' and 'Multivariate Polynomial Ring in w, x, y, z over Finite Field of size 2'

I'm going to push these (non functional) commits now and I'll have a play around soon.

[Bruno-TT]

I've been trying to debug this and it seems that the MPolynomial object's _get_action_ method is never even called

I've looked inside coerce.pyx and it seems we have a situation where the

action = self._action_maps.get(xp, yp, op)

line is raising a KeyError, and the

action = self.get_action(xp, yp, op, x, y)

line within the except statement is returning None

I don't (yet) have a good understanding of the coercion logic, but hopefully this might give a few clues to anyone who understands it better than me.

If not, I'll keep playing around and trying to get the logic to work - perhaps an entry is missing from the _action_maps object?

[tscrim]

Looking at the code you currently have pushed, it seems like you have the wrong signature for _get_action_(), as it should be (in a Cython file):

cpdef _get_action_(self, S, op, bint self_on_left):

So it is being called, but an error is being raised, which is then absorbed by the coercion framework (and then it proceeds on). It's a Python implementation, but see groups/braid.py for an example.

[Bruno-TT]

Hi, I've tried pushing the changes but it still doesn't work.

Also slightly modified the body code (isinstance doesn't work with MatrixGroup)

However the function still isn't being called - there's a print statement at the start that doesn't ever get executed.

[tscrim]

I see the problem; sorry for not noticing this earlier. You put it with the element, but it should go with the parent. This is why you needed to update the pxd file; that isn't necessary for subclasses (as it is inherited). If you put this in

sage.rings.polynomial.multi_polynomial_ring_base.MPolynomialRing_base

then it works for me. This also made me notice that you have to change

-    return super()
+    return super(MPolynomialRing_base, self)._get_action_(G, op, self_on_left)

as we have to be explicit here because of the cpdef and Cython; make the isinstance(G, (MatrixSpace, MatrixGroup_base)); and have the action take a is_left argument which is not self_on_left in the _get_action_() (for this, you probably want to do something more or restrict it to one side). Then I get stuck at a NotImplementedError, but I think that should be easy enough to fix.

Unfortunately I can't easily push changes to your branch with GH. >.< Hopefully my comments are sufficiently clear about what to do. Let me know if you have questions.

[Bruno-TT]

Got it working now! Thanks @tscrim

[tscrim]

Great! No problem Next is the usual details:

• Remove the changes to polynomial/multi_polynomial_libsingular.pyx.
• You don't need to change any pxd files as the declaration is included by inheritance.
• PEP8 spacing; mostly around =.
• Add documentations and tests.

[tscrim]

@Bruno-TT All of the changes in my previous comment need to be addressed before this PR will be approved.

[Bruno-TT]

@tscrim I think they've all been addressed now.

I've added tests, but I'm a little lost as to what documentation can be added; get_action and MatrixPolynomialAction are pretty self documenting.

[tscrim]

Thanks.

A short one-line description of something, even if self-documenting, can still be useful. Plus Sage requires all methods have at least one doctest for all new code.

[tscrim]

Still a few more little things to do, which also includes implementing a __reduce__ method.

[tscrim]

Thanks. You will need doctests for the remaining methods, but that will be the last thing .(It seems like something is also out of sync with GH and the main branch, but I don't think that is anything you can control...)

[Bruno-TT]

Thanks. You will need doctests for the remaining methods, but that will be the last thing .(It seems like something is also out of sync with GH and the main branch, but I don't think that is anything you can control...)

Hi I've written doctests and docstrings

[tscrim]

Thank you. Mostly my comments are formatting changes or places where the doc does not have to be so verbose.

Could we also add a test for a case where we only have coercion of the matrix base ring into the polynomial ring (e.g., a matrix group over ZZ acting on polynomials over QQ)?

[tscrim]

It would be better to check the base ring agreement in the __init__ and fail there. Although I think we only need a coercion from the matrices base ring into the polynomial base ring (e.g., matrices over ZZ have a natural action on polynomials over QQ).

Here, I think we should make sure the matrix is an element of self.MS and similarly for the polynomial. (Although that should already be the case by the time this method is called.)

This assert is also too strong as we only look for coercions of base rings, not equality.

[Bruno-TT]

Hi,

I understand what you mean about moving the check to init, but some of the mathematical formalisms are a bit lost on me regarding the coercion check. I'm assuming I need to call has_coerce_map_from somehow, but I'm not sure what I need to call it on exactly. Any chance you could be a little more specific - my apologies for not understanding this better.

[tscrim]

No problem; there is a lot going on with this.

Compare with the _get_action_(); you will see that you want polynomial.base_ring().has_coerce_map_from(mat.base_ring()) to be True. If you add the doctest I requested, you should see a failure once you do the action (unless I have missed something) before doing this change.

You can remove the assert afterwards, although if you really want a safety check (which really should not be needed at all) is mat.parent() is self.MS and polynomial.parent() is self.PR. I wouldn't add it though.

[Bruno-TT]

ok I've added

assert PR.has_coerce_map_from(MS)

to the start of __init__ and now it's throwing errors - assuming I've misunderstood somehow?

[tscrim]

It should be the base rings:

assert PR.base_ring().has_coerce_map_from(MS.base_ring())

[tscrim]

First, please do not use the generic commit messages for each suggested change (I guess this is something from GH). It makes the history a mess and very difficult to check which thing comes from what change. Please squash all of those into one commit with at least a little less generic message (e.g., "reviewer fixes" is fine with me). (I should have mentioned this on one of the previous rounds, but there were sufficiently many changes that I just did a review from scratch.)

Second, please don't just make the change but look at the comments. You now hove lots of indentation issues that you (still) need to fix.

Also, please follow PEP8 in your doctests, so @ and = should have a spare around them (except for, e.g., f(x=2)).

[github-actions]

Documentation preview for this PR (built with commit 468bb60; changes) is ready! 🎉