Opened 2 years ago

Closed 2 years ago

#22091 closed defect (fixed)

Allow coercion from matrix groups to matrix rings

Reported by: pbruin Owned by:
Priority: major Milestone: sage-7.6
Component: coercion Keywords: matrix
Cc: Merged in:
Authors: Peter Bruin Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 9dec049 (Commits) Commit: 9dec049a056eb5d3b2781070151dd387a46dd3a5
Dependencies: #22128 Stopgaps:

Description (last modified by pbruin)

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).

Change History (17)

comment:1 Changed 2 years ago by pbruin

  • Branch set to u/pbruin/22091-matrix_coercion
  • Commit set to 231eb328fe409443347a40c447789d3cdfc689d7
  • Status changed from new to needs_review

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

comment:2 Changed 2 years ago by pbruin

  • Description modified (diff)

comment:3 follow-up: Changed 2 years ago by vdelecroix

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

Last edited 2 years ago by vdelecroix (previous) (diff)

comment:4 Changed 2 years ago by vdelecroix

Does it fix #18258?

comment:5 in reply to: ↑ 3 ; follow-up: Changed 2 years ago by pbruin

Replying to vdelecroix:

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 vdelecroix:

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.

comment:6 in reply to: ↑ 5 Changed 2 years ago by pbruin

Replying to pbruin:

Replying to vdelecroix:

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.

comment:7 Changed 2 years ago by pbruin

  • Dependencies set to #22128

The above bug is now #22128.

comment:8 Changed 2 years ago by git

  • Commit changed from 231eb328fe409443347a40c447789d3cdfc689d7 to f36872025a52fc0b70c1a7a9845c4a0a2b556a50

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

5afe081Trac 22128: fix pickling of FinitelyGeneratedMatrixGroup_generic
f368720Trac 22091: allow coercion from matrix groups to matrix rings

comment:9 follow-up: Changed 2 years ago by 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). While I cannot see the only branch because you forced-push, I suspect the previous version was a better approach.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 2 years ago by pbruin

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.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 2 years ago by vdelecroix

Replying to pbruin:

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.

comment:12 in reply to: ↑ 11 Changed 2 years ago by tscrim

Replying to vdelecroix:

Replying to pbruin:

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.

comment:13 Changed 2 years ago by git

  • Commit changed from f36872025a52fc0b70c1a7a9845c4a0a2b556a50 to 9dec049a056eb5d3b2781070151dd387a46dd3a5

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

9dec049Trac 22091: allow coercion from matrix groups to matrix rings

comment:14 Changed 2 years ago by pbruin

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.

comment:15 Changed 2 years ago by pbruin

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

comment:16 Changed 2 years ago by tscrim

  • Milestone changed from sage-7.5 to sage-7.6
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM, so I'm setting a positive review.

comment:17 Changed 2 years ago by vbraun

  • Branch changed from u/pbruin/22091-matrix_coercion to 9dec049a056eb5d3b2781070151dd387a46dd3a5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.