Allow coercion from matrix groups to matrix rings

[pjbruin]

If A is a ring, then the matrix group GL(n, A) does not have a coercion map to the ring MatrixSpace(A, n) of which it is the unit group:

sage: A = Zmod(4)
sage: R = MatrixSpace(A, 2)
sage: G = GL(2, A)
sage: R.has_coerce_map_from(G)
False
sage: m = R([[1, 0], [0, 1]])
sage: m in G
True
sage: m in list(G)
False
sage: m == G(m)
False

All answers should be True. The fact that m in G returns the correct answer despite this bug is the result of a non-standard implementation of __contains__(); see #22092 (of which this ticket is a dependency).

Depends on #22128

Component: coercion

Keywords: matrix

Author: Peter Bruin

Branch/Commit: 9dec049

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/22091

[pjbruin]

Commit: 231eb32

[pjbruin]

Branch: u/pbruin/22091-matrix_coercion

[pjbruin]

comment:1

The criterion used in the patch was copied from the method MatrixSpace.matrix(). Note that this still uses the old-style coercion model.

[videlec]

comment:3

I do think that declaring a coercion embedding would be more appropriate.

[videlec]

comment:4

Does it fix #18258?

[pjbruin]

comment:5

Replying to @videlec:

I do think that declaring a coercion embedding would be more appropriate.

You're probably right. I changed this and am now running doctests.

Replying to @videlec:

Does it fix #18258?

Yes, it does. Maybe we should fix the actual bug on this ticket (by adding the coercion embedding) and keep #18258 for the other fixes (upgrading MatrixSpace to use the "new" coercion system). I will add doctests for the examples from #18258.

[pjbruin]

comment:6

Replying to @pjbruin:

Replying to @videlec:

I do think that declaring a coercion embedding would be more appropriate.

You're probably right. I changed this and am now running doctests.

There was one doctest failure (in src/sage/groups/matrix_gps/finitely_generated.py), which uncovered the following existing bug:

A = SR
R = MatrixSpace(A, 2)
m = R([[1,1],[0,1]])
G = MatrixGroup([m])
G.register_embedding(R)  # declare coercion embedding
loads(dumps(G))
Traceback (most recent call last):
...
AttributeError: 'NoneType' object has no attribute 'homset_category'

The error also arises with A = RR or CC, but not with A = ZZ, QQ or GF(17), for example; I guess this is because a different (GAP-based) class is used in the latter cases.

[pjbruin]

Dependencies: #22128

[pjbruin]

comment:7

The above bug is now #22128.

[sagetrac-git]

Changed commit from 231eb32 to f368720

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5afe081	Trac 22128: fix pickling of FinitelyGeneratedMatrixGroup_generic
f368720	Trac 22091: allow coercion from matrix groups to matrix rings

[tscrim]

comment:9

A better fix long-term is to actually to _coerce_map_from_ (once MatrixSpace is a Parent). That being said, I disagree with using the (only one) coerce embedding of the matrix group as it takes away that limited resource from the user and we have more general approaches, even with ParentWithGens. It also becomes a single point in the code where the coercions are handled, making maintenance and the logic easier (including when switching to Parent/new coercion system). While I cannot see the only branch because you forced-push, I suspect the previous version was a better approach.

[pjbruin]

comment:10

Replying to @tscrim:

A better fix long-term is to actually to _coerce_map_from_ (once MatrixSpace is a Parent). That being said, I disagree with using the (only one) coerce embedding of the matrix group as it takes away that limited resource from the user and we have more general approaches, even with ParentWithGens. It also becomes a single point in the code where the coercions are handled, making maintenance and the logic easier (including when switching to Parent/new coercion system).

I see your point; I do find the new solution more elegant than the old one, but this might partly be because MatrixSpace still uses the old coercion system.

About the possibility for the user to profit from register_embedding() being available: users should not count on being able to do this, as the same group may well have been constructed earlier in the same Sage session and have been used in the coercion system. In this case register_embedding() will fail.

Anyway, I only have a slight preference for the new solution. Vincent, what are your reasons for preferring this solution?

While I cannot see the only branch because you forced-push, I suspect the previous version was a better approach.

The old version is commit 231eb32.

[videlec]

comment:11

Replying to @pjbruin:

Replying to @tscrim:

A better fix long-term is to actually to _coerce_map_from_ (once MatrixSpace is a Parent). That being said, I disagree with using the (only one) coerce embedding of the matrix group as it takes away that limited resource from the user and we have more general approaches, even with ParentWithGens. It also becomes a single point in the code where the coercions are handled, making maintenance and the logic easier (including when switching to Parent/new coercion system).

I see your point; I do find the new solution more elegant than the old one, but this might partly be because MatrixSpace still uses the old coercion system.

I don't understand the logic. Travis, you are proposing to declare the coercion in the MatrixSpace code?

About the possibility for the user to profit from register_embedding() being available: users should not count on being able to do this, as the same group may well have been constructed earlier in the same Sage session and have been used in the coercion system. In this case register_embedding() will fail.

+1. This has been proved to be confusing and broken (#19016, #15303, #19388).

Anyway, I only have a slight preference for the new solution. Vincent, what are your reasons for preferring this solution?

Looks more (mathematically) natural to me. When I define a subset Y of X I want to set a coerce embedding from Y to X. I might be wrong.

I also prefer having the embedding declared once for all in init. There could be an option in the constructor to have this embedding. I would follow what is done for number fields: the embedding is part of the input data to construct it.

[tscrim]

comment:12

Replying to @videlec:

Replying to @pjbruin:

Replying to @tscrim:

A better fix long-term is to actually to _coerce_map_from_ (once MatrixSpace is a Parent). That being said, I disagree with using the (only one) coerce embedding of the matrix group as it takes away that limited resource from the user and we have more general approaches, even with ParentWithGens. It also becomes a single point in the code where the coercions are handled, making maintenance and the logic easier (including when switching to Parent/new coercion system).

I see your point; I do find the new solution more elegant than the old one, but this might partly be because MatrixSpace still uses the old coercion system.

I don't understand the logic. Travis, you are proposing to declare the coercion in the MatrixSpace code?

Yes, that is where it should go by our long-standing API for the coercion framework. Using register_embedding(), you are doing something using a dynamic (and as you mention below, somewhat fragile) method instead of the standard, well-used, and static/method-based handles to doing coercions.

About the possibility for the user to profit from register_embedding() being available: users should not count on being able to do this, as the same group may well have been constructed earlier in the same Sage session and have been used in the coercion system. In this case register_embedding() will fail.

+1. This has been proved to be confusing and broken (#19016, #15303, #19388).

Then we should avoid it. By also defining an embedding during construction, you've created a unique situation in Sage, which will further confuse users, and now likely experts.

Anyway, I only have a slight preference for the new solution. Vincent, what are your reasons for preferring this solution?

Looks more (mathematically) natural to me. When I define a subset Y of X I want to set a coerce embedding from Y to X. I might be wrong.

This is close to a fallacy to me, as you are not construction Y as a subset of X. Besides, you also want X to know that Y was constructed as a subset of itself. While both viewpoints are valid, in Sage, we essentially decided on the latter one by how we have defined the coercion system.

I also prefer having the embedding declared once for all in init. There could be an option in the constructor to have this embedding. I would follow what is done for number fields: the embedding is part of the input data to construct it.

IMO, this is even better: it is being hard-coded into the class itself, where you do not even need to define the object. Number fields are a different scenario by their construction, but I would still even argue that we should use the standard approach.

[sagetrac-git]

Changed commit from f368720 to 9dec049

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

9dec049

Trac 22091: allow coercion from matrix groups to matrix rings

[pjbruin]

comment:14

I reverted to the original approach (with extra doctests from the other version). The argument that the approach of declaring coercion embeddings is non-standard and fundamentally slightly fragile does sound convincing to me. That having been said, I would simply like to see this bug fixed, no matter how exactly.

[pjbruin]

comment:15

Note: the dependency #22128 is marked as "closed" but does not seem to be merged in 7.6.beta0 yet.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:16

LGTM, so I'm setting a positive review.

[vbraun]

Changed branch from u/pbruin/22091-matrix_coercion to 9dec049