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

structure/coerce actions #37916

Merged
merged 1 commit into from
May 12, 2024
Merged

Conversation

mantepse
Copy link
Contributor

@mantepse mantepse commented May 1, 2024

We modify ModuleAction to prefer coercion of the base ring over creating an action. More precisely, before this branch, we have

sage: G = PolynomialRing(QQ, "x")
sage: S = PolynomialRing(MatrixSpace(QQ, 2), "x")
sage: G.gen() * S.gen()
[x 0]
[0 x]*x

instead of

            [1 0]
            [0 1]*x^2

and

sage: G = PolynomialRing(QQ, "x")
sage: S = PolynomialRing(InfinitePolynomialRing(QQ, "a"), "x")
sage: G.gen() * S.an_element()
x*x

instead of

x^2

which is at least surprising.

In the end, this is needed to make #37033 work.

Authors: @tscrim and @mantepse.

Copy link

github-actions bot commented May 1, 2024

Documentation preview for this PR (built with commit b9947d2; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mantepse mantepse force-pushed the structure/coerce_actions branch from f3f2bf8 to b9947d2 Compare May 1, 2024 20:18
@nbruin
Copy link
Contributor

nbruin commented May 1, 2024

You might want to be careful with a change like that. Mathematically, for instance, a ZZ-algebra with unity is a ZZ-module with extra structure, so the action of ZZ on the algebra is more fundamental than the multiplication structure on the algebra itself. So mathematically, it makes sense to prefer the ZZ-action over coercion from ZZ into the algebra followed by multiplication.

For performance, this can also be a difference: multiplying by an integer (i.e., a scalar multiplication) is really quite a bit cheaper than using full algebra multiplication with an element that happens to be a coerced scalar.

Thank about the situation where you have the full n x n matrix algebra over a field k. Then k coerces into the algebra as diagonal matrices. Would your change prefer that coercion over the scalar multiplication action? Multiplying two n x n matrices is significantly more expensive than a scalar multiplication of a matrix.

Note that the matrix algebra is simply an example. Perhaps there things are arranged in such a way that scalar multiplication gets recognized early or in a different way. But the principle applies in many cases. It may well be that your change is fine, but you should carefully review that it is, certainly for the reason above.

@tscrim
Copy link
Collaborator

tscrim commented May 1, 2024

Martin’s description is oversimplified and missing a key component. We do not prefer coercion over already defined actions for the reasons you stated (performance and being natural). However, we instead prefer doing coercion over doing a pushout of the base ring.

@tscrim
Copy link
Collaborator

tscrim commented May 1, 2024

The situation at hand is this. Suppose there is a coercion $\phi \colon R \to S$, where $S$ is an $F$-algebra. What the pushout construction does in $r \cdot s$ (via r * s) is create $S’$ as an extension of scalars of $S$ to an $R$-algebra. However, the coercion map should mean instead treat $r$ as $\phi(r) \in S$ to make sense of the multiplication instead of the natural $R$-action on $S’$. This agrees with what common_parent would find too.

@nbruin
Copy link
Contributor

nbruin commented May 2, 2024

Oh thanks. If you put it that way it sounds quite natural to do.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Than I will approve this.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 9, 2024
sagemathgh-37916: structure/coerce actions
    
We modify `ModuleAction` to prefer coercion of the base ring over
creating an action.  More precisely, before this branch, we have

```
sage: G = PolynomialRing(QQ, "x")
sage: S = PolynomialRing(MatrixSpace(QQ, 2), "x")
sage: G.gen() * S.gen()
[x 0]
[0 x]*x
```
instead of
```
            [1 0]
            [0 1]*x^2
```
and
```
sage: G = PolynomialRing(QQ, "x")
sage: S = PolynomialRing(InfinitePolynomialRing(QQ, "a"), "x")
sage: G.gen() * S.an_element()
x*x
```
instead of
```
x^2
```
which is at least surprising.

In the end, this is needed to make sagemath#37033 work.

Authors: @tscrim and @mantepse.
    
URL: sagemath#37916
Reported by: Martin Rubey
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
sagemathgh-37916: structure/coerce actions
    
We modify `ModuleAction` to prefer coercion of the base ring over
creating an action.  More precisely, before this branch, we have

```
sage: G = PolynomialRing(QQ, "x")
sage: S = PolynomialRing(MatrixSpace(QQ, 2), "x")
sage: G.gen() * S.gen()
[x 0]
[0 x]*x
```
instead of
```
            [1 0]
            [0 1]*x^2
```
and
```
sage: G = PolynomialRing(QQ, "x")
sage: S = PolynomialRing(InfinitePolynomialRing(QQ, "a"), "x")
sage: G.gen() * S.an_element()
x*x
```
instead of
```
x^2
```
which is at least surprising.

In the end, this is needed to make sagemath#37033 work.

Authors: @tscrim and @mantepse.
    
URL: sagemath#37916
Reported by: Martin Rubey
Reviewer(s): Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
sagemathgh-37916: structure/coerce actions
    
We modify `ModuleAction` to prefer coercion of the base ring over
creating an action.  More precisely, before this branch, we have

```
sage: G = PolynomialRing(QQ, "x")
sage: S = PolynomialRing(MatrixSpace(QQ, 2), "x")
sage: G.gen() * S.gen()
[x 0]
[0 x]*x
```
instead of
```
            [1 0]
            [0 1]*x^2
```
and
```
sage: G = PolynomialRing(QQ, "x")
sage: S = PolynomialRing(InfinitePolynomialRing(QQ, "a"), "x")
sage: G.gen() * S.an_element()
x*x
```
instead of
```
x^2
```
which is at least surprising.

In the end, this is needed to make sagemath#37033 work.

Authors: @tscrim and @mantepse.
    
URL: sagemath#37916
Reported by: Martin Rubey
Reviewer(s): Travis Scrimshaw
@vbraun vbraun merged commit 70a42aa into sagemath:develop May 12, 2024
20 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 12, 2024
@mantepse mantepse deleted the structure/coerce_actions branch May 13, 2024 07:29
@mantepse mantepse restored the structure/coerce_actions branch May 13, 2024 07:29
@mantepse mantepse deleted the structure/coerce_actions branch May 13, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants