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:  sage7.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 )
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 nonstandard implementation of __contains__()
; see #22092 (of which this ticket is a dependency).
Change History (17)
comment:1 Changed 2 years ago by
 Branch set to u/pbruin/22091matrix_coercion
 Commit set to 231eb328fe409443347a40c447789d3cdfc689d7
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
 Description modified (diff)
comment:3 followup: ↓ 5 Changed 2 years ago by
I do think that declaring a coercion embedding would be more appropriate.
comment:4 Changed 2 years ago by
Does it fix #18258?
comment:5 in reply to: ↑ 3 ; followup: ↓ 6 Changed 2 years ago by
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
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 (GAPbased) class is used in the latter cases.
comment:8 Changed 2 years ago by
 Commit changed from 231eb328fe409443347a40c447789d3cdfc689d7 to f36872025a52fc0b70c1a7a9845c4a0a2b556a50
comment:9 followup: ↓ 10 Changed 2 years ago by
A better fix longterm 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 forcedpush, I suspect the previous version was a better approach.
comment:10 in reply to: ↑ 9 ; followup: ↓ 11 Changed 2 years ago by
Replying to tscrim:
A better fix longterm is to actually to
_coerce_map_from_
(onceMatrixSpace
is aParent
). 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 withParentWithGens
. It also becomes a single point in the code where the coercions are handled, making maintenance and the logic easier (including when switching toParent
/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 forcedpush, I suspect the previous version was a better approach.
The old version is commit 231eb32.
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 2 years ago by
Replying to pbruin:
Replying to tscrim:
A better fix longterm is to actually to
_coerce_map_from_
(onceMatrixSpace
is aParent
). 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 withParentWithGens
. It also becomes a single point in the code where the coercions are handled, making maintenance and the logic easier (including when switching toParent
/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 caseregister_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
Replying to vdelecroix:
Replying to pbruin:
Replying to tscrim:
A better fix longterm is to actually to
_coerce_map_from_
(onceMatrixSpace
is aParent
). 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 withParentWithGens
. It also becomes a single point in the code where the coercions are handled, making maintenance and the logic easier (including when switching toParent
/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 longstanding 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, wellused, and static/methodbased 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 caseregister_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
ofX
I want to set a coerce embedding fromY
toX
. 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 hardcoded 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
 Commit changed from f36872025a52fc0b70c1a7a9845c4a0a2b556a50 to 9dec049a056eb5d3b2781070151dd387a46dd3a5
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

comment:14 Changed 2 years ago by
I reverted to the original approach (with extra doctests from the other version). The argument that the approach of declaring coercion embeddings is nonstandard 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
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
 Milestone changed from sage7.5 to sage7.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
 Branch changed from u/pbruin/22091matrix_coercion to 9dec049a056eb5d3b2781070151dd387a46dd3a5
 Resolution set to fixed
 Status changed from positive_review to closed
The criterion used in the patch was copied from the method
MatrixSpace.matrix()
. Note that this still uses the oldstyle coercion model.