Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New PR for action of MatrixGroup on a MPolynomialRing #35611
base: develop
Are you sure you want to change the base?
New PR for action of MatrixGroup on a MPolynomialRing #35611
Changes from 27 commits
20d30f1
3f56030
61c0aa9
bfd6f4a
0e210d8
bd83e57
96b8446
e88c35f
7d0a240
4608cb6
65ba9fd
2ff7643
aa0d722
c746e0e
cc2fea5
9d30130
162e248
bc91221
01e9de3
1ca6a20
2398542
e3d1801
8e858c5
a2c1a8a
44333cb
7892e72
6a5779f
59401d7
a30b0bc
864ede7
5e88a54
b1ccf18
95cfadc
4ea3533
cfd7b20
5f4dbca
468bb60
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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.
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.
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.
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.
No problem; there is a lot going on with this.
Compare with the
_get_action_()
; you will see that you wantpolynomial.base_ring().has_coerce_map_from(mat.base_ring())
to beTrue
. 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) ismat.parent() is self.MS and polynomial.parent() is self.PR
. I wouldn't add it though.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.
ok I've added
to the start of
__init__
and now it's throwing errors - assuming I've misunderstood somehow?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 should be the base rings: