#19225 closed defect (fixed)
cartesian product of algebra has troubles with base_ring
Reported by: | dkrenn | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.1 |
Component: | coercion | Keywords: | |
Cc: | roed, behackl, pbruin, nthiery | Merged in: | |
Authors: | Peter Bruin | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | dcc25ea (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | #29225 | Stopgaps: |
Description (last modified by )
sage: cartesian_product((QQ['z'],)) Traceback (most recent call last) ... /local/dakrenn/sage/6.9.beta6/local/lib/python2.7/site-packages/sage/categories/unital_algebras.pyc in __init_extra__(self) --> 107 H = Hom(base_ring, self, Rings()) # TODO: non associative ring! ... AttributeError: 'NoneType' object has no attribute '_is_category_initialized'
Change History (31)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
- Cc behackl added
- Description modified (diff)
comment:4 follow-up: ↓ 5 Changed 5 years ago by
- Branch set to u/pbruin/19225-cartesian_product_base
- Commit set to 2359b7201ed112fb18d41c9e89ae86080d9db960
- Component changed from categories to coercion
- Milestone changed from sage-6.9 to sage-8.1
- Status changed from new to needs_review
There are several related problems here, and it seems best to solve them together:
- the error described in the ticket description
- after fixing this in a minimal way, the base ring of the Cartesian product in the example is not initialised
- after fixing this as well, coercion from the base ring is not implemented.
The attached branch fixes the above problems essentially by getting rid of the method __init_extra__
of UnitalAlgebras
, which used the somewhat problematic register_coercion()
mechanism, and implementing equivalent functionality in _coerce_map_from_()
instead. Because this method can (and is) overridden by other _coerce_map_from_()
methods, those need to be adapted. Depending on the implementation of coercion from the base ring, this is done either through super
(resulting in a coercion map implemented via _lmul_()
, as in the old UnitalAlgebras.__init_extra__()
) or by returning True
(resulting in a DefaultConvertMap
).
Some occurrences of x.parent()
were changed to parent(x)
in cases where x
can be a Python type. Also, there was a bidirectional coercion between free algebras and their "PBW bases" which caused doctest failures; the coercion from the free algebra to the PBW bases was not needed and has been removed. Finally, this allows the hack introduced in #11900 (an attribute _no_generic_basering_coercion
) can be removed, and seems to solve #16492.
comment:5 in reply to: ↑ 4 ; follow-ups: ↓ 6 ↓ 7 ↓ 8 ↓ 9 ↓ 10 Changed 5 years ago by
- Status changed from needs_review to needs_work
Replying to pbruin:
There are several related problems here, and it seems best to solve them together:
- the error described in the ticket description
- after fixing this in a minimal way, the base ring of the Cartesian product in the example is not initialised
- after fixing this as well, coercion from the base ring is not implemented.
The attached branch fixes the above problems essentially by getting rid of the method
__init_extra__
ofUnitalAlgebras
, which used the somewhat problematicregister_coercion()
mechanism, and implementing equivalent functionality in_coerce_map_from_()
instead. Because this method can (and is) overridden by other_coerce_map_from_()
methods, those need to be adapted. Depending on the implementation of coercion from the base ring, this is done either throughsuper
(resulting in a coercion map implemented via_lmul_()
, as in the oldUnitalAlgebras.__init_extra__()
) or by returningTrue
(resulting in aDefaultConvertMap
).
-1 to getting rid of this mechanism. It means you can no longer simply inherit from, e.g., CombinatorialFreeModule
and set the category when creating a new (unital) algebra. It also acts as a safeguard to do something that every unital algebra should do.
Perhaps an improvement would be for __init_extra__
to first check to see if the usual _coerce_map_from_
does result in a coercion from the base ring. At least this would fix the need for the _no_generic_basering_coercion
in polynomial rings. I haven't checked to see if this will fix the problem in the ticket description.
Also, technically, an implementation of _lmul_
is not a coercion, but an action (maybe with a coercion into the domain). From what I remember, I don't see a way this can be done within the coercion framework, which first tries to make both elements into the same parent and then do multiplication within that parent.
Some occurrences of
x.parent()
were changed toparent(x)
in cases wherex
can be a Python type.
I've been told that this is faster too.
Also, there was a bidirectional coercion between free algebras and their "PBW bases" which caused doctest failures; the coercion from the free algebra to the PBW bases was not needed and has been removed.
This is an immediate -1 from me because they are the same object (in the mathematical sense), just expressed in terms of different bases. So there should, and needs, to be a bidirectional coercion. The fact that this needs to be removed is a very strong indication to me that this is not the correct fix.
Finally, this allows the hack introduced in #11900 (an attribute
_no_generic_basering_coercion
) can be removed, and seems to solve #16492.
However, the fact that _coerce_map_from_
returns True
is a definite bug as everything (supposedly) coerces in. I do not believe it is something that _element_constructor_
can dictate.
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 13 Changed 5 years ago by
Replying to tscrim:
Replying to pbruin:
There are several related problems here, and it seems best to solve them together:
- the error described in the ticket description
- after fixing this in a minimal way, the base ring of the Cartesian product in the example is not initialised
- after fixing this as well, coercion from the base ring is not implemented.
The attached branch fixes the above problems essentially by getting rid of the method
__init_extra__
ofUnitalAlgebras
, which used the somewhat problematicregister_coercion()
mechanism, and implementing equivalent functionality in_coerce_map_from_()
instead. Because this method can (and is) overridden by other_coerce_map_from_()
methods, those need to be adapted. Depending on the implementation of coercion from the base ring, this is done either throughsuper
(resulting in a coercion map implemented via_lmul_()
, as in the oldUnitalAlgebras.__init_extra__()
) or by returningTrue
(resulting in aDefaultConvertMap
).-1 to getting rid of this mechanism. It means you can no longer simply inherit from, e.g.,
CombinatorialFreeModule
and set the category when creating a new (unital) algebra.
This is not true; if you do this, then everything should work in the same way as before except the default coercion map is now discovered by _coerce_map_from_()
. Of course, if you override _coerce_map_from_()
then you either have to make sure you return a custom coercion map from the base ring (if you have one), or call super
to get the default one, implemented via _lmul_()
. I am convinced that this is a feature, not a bug, and that the existing method has several disadvantages:
- because the coercion map is constructed in
__init_extra__()
, it is always constructed, even if you are never going to use it; - as a consequence, if a user wants to declare a coercion map to or from an algebra, this is impossible because the coercion system has already been invoked;
- similarly, when writing a new class of algebras (in my case I was trying to build one on top of Cartesian products of algebras), and you want to use your own coercion from the base ring, you have to resort to the ugly
_no_generic_basering_coercion
hack.
comment:7 in reply to: ↑ 5 ; follow-up: ↓ 11 Changed 5 years ago by
Replying to tscrim:
Perhaps an improvement would be for
__init_extra__
to first check to see if the usual_coerce_map_from_
does result in a coercion from the base ring. At least this would fix the need for the_no_generic_basering_coercion
in polynomial rings. I haven't checked to see if this will fix the problem in the ticket description.
By definition, when __init_extra__
runs the parent hasn't been fully initialised yet, so this sounds fragile to me. My impression is that the problem that such a construction would solve is solved in a more standard and flexible way by using super
in _coerce_map_from_()
if necessary.
comment:8 in reply to: ↑ 5 ; follow-up: ↓ 12 Changed 5 years ago by
Replying to tscrim:
Also, technically, an implementation of
_lmul_
is not a coercion, but an action (maybe with a coercion into the domain). From what I remember, I don't see a way this can be done within the coercion framework, which first tries to make both elements into the same parent and then do multiplication within that parent.
If you look at the code of UnitalAlgebras.ParentMethods.__init_extra__()
, you will see that the default coercion map from the base ring is implemented as an action via _lmul_()
. The coercion framework does discover and use actions, not just coercion into a common parent. For example:
sage: cm = get_coercion_model() sage: R.<x> = QQ[] sage: cm.explain(1, x) Action discovered. Left scalar multiplication by Integer Ring on Univariate Polynomial Ring in x over Rational Field Result lives in Univariate Polynomial Ring in x over Rational Field Univariate Polynomial Ring in x over Rational Field
comment:9 in reply to: ↑ 5 ; follow-up: ↓ 14 Changed 5 years ago by
Replying to tscrim:
Also, there was a bidirectional coercion between free algebras and their "PBW bases" which caused doctest failures; the coercion from the free algebra to the PBW bases was not needed and has been removed.
This is an immediate -1 from me because they are the same object (in the mathematical sense), just expressed in terms of different bases. So there should, and needs, to be a bidirectional coercion. The fact that this needs to be removed is a very strong indication to me that this is not the correct fix.
Bidirectional coercions should always be avoided, for at least two reasons:
- If
X
andY
are two parents with bidirectional coercion maps, and ifx
andy
are elements ofX
andY
, respectively, then it is not clear wherex + y
should live. - Bidirectional coercion implies memory leak.
comment:10 in reply to: ↑ 5 Changed 5 years ago by
Replying to tscrim:
However, the fact that
_coerce_map_from_
returnsTrue
is a definite bug as everything (supposedly) coerces in. I do not believe it is something that_element_constructor_
can dictate.
That is true; I was careless here and will fix that.
comment:11 in reply to: ↑ 7 Changed 5 years ago by
Replying to pbruin:
Replying to tscrim:
Perhaps an improvement would be for
__init_extra__
to first check to see if the usual_coerce_map_from_
does result in a coercion from the base ring. At least this would fix the need for the_no_generic_basering_coercion
in polynomial rings. I haven't checked to see if this will fix the problem in the ticket description.By definition, when
__init_extra__
runs the parent hasn't been fully initialised yet, so this sounds fragile to me. My impression is that the problem that such a construction would solve is solved in a more standard and flexible way by usingsuper
in_coerce_map_from_()
if necessary.
It is the last thing run by Parent.__init__
, so everything is properly initialized (at least that matters for this). So it should be robust. It also more strongly tries to enforce that an object in this category has certain features.
I am also very strongly opposed to implementing coercions using super calls within categories. First, there are technical limitations. Second, it can be natural that you do not want to call _coerce_map_from_
going up the MRO because you have extra structure (e.g., you are not a full subcategory). For example, there may be no coercion as graded modules that would be there as modules. Yes, there are ways around that, but they are not as pretty or specific.
You also are likely to get a speed regression when finding if coercions exists because of the extra walk up through the category MRO.
comment:12 in reply to: ↑ 8 Changed 5 years ago by
Replying to pbruin:
Replying to tscrim:
Also, technically, an implementation of
_lmul_
is not a coercion, but an action (maybe with a coercion into the domain). From what I remember, I don't see a way this can be done within the coercion framework, which first tries to make both elements into the same parent and then do multiplication within that parent.If you look at the code of
UnitalAlgebras.ParentMethods.__init_extra__()
, you will see that the default coercion map from the base ring is implemented as an action via_lmul_()
.
I just don't quite agree with the phrasing as it makes it seem like _lmul_
is the coercion, not wrapped in a SetMorphism
. Although IIRC, this is moot because actions has precedence over coercions.
comment:13 in reply to: ↑ 6 Changed 5 years ago by
Replying to pbruin:
Replying to tscrim:
Replying to pbruin:
There are several related problems here, and it seems best to solve them together:
- the error described in the ticket description
- after fixing this in a minimal way, the base ring of the Cartesian product in the example is not initialised
- after fixing this as well, coercion from the base ring is not implemented.
The attached branch fixes the above problems essentially by getting rid of the method
__init_extra__
ofUnitalAlgebras
, which used the somewhat problematicregister_coercion()
mechanism, and implementing equivalent functionality in_coerce_map_from_()
instead. Because this method can (and is) overridden by other_coerce_map_from_()
methods, those need to be adapted. Depending on the implementation of coercion from the base ring, this is done either throughsuper
(resulting in a coercion map implemented via_lmul_()
, as in the oldUnitalAlgebras.__init_extra__()
) or by returningTrue
(resulting in aDefaultConvertMap
).-1 to getting rid of this mechanism. It means you can no longer simply inherit from, e.g.,
CombinatorialFreeModule
and set the category when creating a new (unital) algebra.This is not true; if you do this, then everything should work in the same way as before except the default coercion map is now discovered by
_coerce_map_from_()
.
Ah, right. Sorry, I missed the change in module.pyx
.
Of course, if you override
_coerce_map_from_()
then you either have to make sure you return a custom coercion map from the base ring (if you have one), or callsuper
to get the default one, implemented via_lmul_()
. I am convinced that this is a feature, not a bug,
I never claimed this was a bug. However, I am worried about speed regressions here from walking up the MRO, as well as enforcing it could lead to some mathematical issues because possible (natural) coercion is inversely proportional to the amount of structure. See my other reply.
and that the existing method has several disadvantages:
- because the coercion map is constructed in
__init_extra__()
, it is always constructed, even if you are never going to use it;
I see this as an enforcement of the category requirements. However, it does have a cost when constructing the parents, and I agree that it should be avoiding.
- as a consequence, if a user wants to declare a coercion map to or from an algebra, this is impossible because the coercion system has already been invoked;
No, that is not true. You are adding a coercion map. So you can always add additional coercions. With develop
:
sage: C = CombinatorialFreeModule(QQ, ['a','b']) sage: D = CombinatorialFreeModule(QQ, ['a','b','c']) sage: phi = C.module_morphism(D.basis().__getitem__, codomain=D) sage: C.register_embedding(phi) sage: D(C.basis()['a']).parent() Free module generated by {'a', 'b', 'c'} over Rational Field
It only freezes register_embedding
when you invoke the coercion framework directly:
sage: psi = C.module_morphism(E.basis().__getitem__, codomain=E) sage: psi.register_as_coercion()
- similarly, when writing a new class of algebras (in my case I was trying to build one on top of Cartesian products of algebras), and you want to use your own coercion from the base ring, you have to resort to the ugly
_no_generic_basering_coercion
hack.
In some ways that is a workaround, but it also gives you finer control.
comment:14 in reply to: ↑ 9 Changed 5 years ago by
Replying to pbruin:
Replying to tscrim:
Also, there was a bidirectional coercion between free algebras and their "PBW bases" which caused doctest failures; the coercion from the free algebra to the PBW bases was not needed and has been removed.
This is an immediate -1 from me because they are the same object (in the mathematical sense), just expressed in terms of different bases. So there should, and needs, to be a bidirectional coercion. The fact that this needs to be removed is a very strong indication to me that this is not the correct fix.
Bidirectional coercions should always be avoided, for at least two reasons:
- If
X
andY
are two parents with bidirectional coercion maps, and ifx
andy
are elements ofX
andY
, respectively, then it is not clear wherex + y
should live.
IMO, ambiguity is the correct thing. They are different representations of the same parent, and in general, why should one be preferred over the other? For PBW bases here, the natural monomial basis is "better" in some sense, but since you created the special PBW basis, you probably want the PBW. What about for things like symmetric functions or Iwahori-Hecke algebras that have several bases that all sit on equal footing?
Excuse a little theatrics, but my first thought was, "that's horrifying". I believe I understand why this happens, but I feel that is more of a problem of a bad caching mechanism in the coercion framework. It also puts a much heavier burden on someone writing code because you will have to avoid all coercion cycles.
My initial thought is also that you cannot get around this by specifying conversion maps because it puts the other parent as the key. I could see it making the non-coercion direction slower if the inverse map was implemented by inverting one a ModuleMorphismByLinearity
, used caching of elements (this might be a memory leak in and of itself), or some other form of needing to create a morphism. It also would make the code of _element_constructor_
much more complicated with extra cases, especially when a mathematical object has a lot of realizations.
comment:15 Changed 5 years ago by
- Cc nthiery added
comment:16 Changed 2 years ago by
- Dependencies set to #29225
I am now working on a more minimal solution. After splitting off part of it into #29225, I realised that time has progressed by exactly 104 tickets...
comment:17 Changed 2 years ago by
- Branch changed from u/pbruin/19225-cartesian_product_base to u/pbruin/19225-cartesian_product_algebra
- Commit changed from 2359b7201ed112fb18d41c9e89ae86080d9db960 to a040a0e0f9d1e2ce25bf30062f8e9815f9a2215d
- Status changed from needs_work to needs_review
The approach in this new branch is to move the machinery for setting up the coercion from the base ring of an algebra into a separate method _coerce_map_from_base_ring()
. This is called by the existing __init_extra__()
(so it has the same effect as before), but also by a new default _coerce_map_from_()
method. This then registers the coercion in cases where __init_extra__()
runs at a time when not everything is initialised.
comment:18 follow-up: ↓ 20 Changed 2 years ago by
This comment needs to be changed
-# We need to register a coercion from the base ring to self. +# We need to construct a coercion from the base ring to self.
I think your _lmul_
can create elements in the parent that are not actually suppose to be in the parent. Specifically, _cartesian_product_of_elements
does not do any safety checking (nor do I think it should as it is meant to be called internally with safe input). So in your example, if you instead multiplied by 1/5
, I think you get an element of B
that is not in B
.
Do Jordan algebras still need the _no_generic_basering_coercion
? Can we also get rid of it for matrix spaces and polynomial rings?
Also why should we allow for base_ring is None
in __init_extra__
to be valid? I feel like if we get to that point and there is no base ring, then it is a bug (as we have declared the syntax of the base ring must be setup during the __init__
call) and it should result in an error.
comment:19 Changed 2 years ago by
- Commit changed from a040a0e0f9d1e2ce25bf30062f8e9815f9a2215d to dcc25eadbd49dd1af066ffc8f76766745bffa361
Branch pushed to git repo; I updated commit sha1. New commits:
dcc25ea | Trac 19225: update comment
|
comment:20 in reply to: ↑ 18 ; follow-up: ↓ 21 Changed 2 years ago by
Replying to tscrim:
I think your
_lmul_
can create elements in the parent that are not actually suppose to be in the parent. Specifically,_cartesian_product_of_elements
does not do any safety checking (nor do I think it should as it is meant to be called internally with safe input). So in your example, if you instead multiplied by1/5
, I think you get an element ofB
that is not inB
.
It doesn't seem this happens, because _lmul_()
itself is also called with safe input:
sage: A = FreeModule(ZZ, 2) sage: B = cartesian_product([A, A]) sage: 1/5*B(([1, 2], [3, 4])) Traceback (most recent call last): ... TypeError: unsupported operand parent(s) for *: 'Rational Field' and 'The Cartesian product of (Ambient free module of rank 2 over the principal ideal domain Integer Ring, Ambient free module of rank 2 over the principal ideal domain Integer Ring)'
Do Jordan algebras still need the
_no_generic_basering_coercion
? Can we also get rid of it for matrix spaces and polynomial rings?
Yes, but I prefer to do this on a separate ticket. For Jordan algebras it is trivial, for matrix algebras it requires adding a one-line _coerce_map_from_base_ring()
method, but for polynomial rings it is somewhat less trivial.
Also why should we allow for
base_ring is None
in__init_extra__
to be valid? I feel like if we get to that point and there is no base ring, then it is a bug (as we have declared the syntax of the base ring must be setup during the__init__
call) and it should result in an error.
The error reported in this ticket occurs inside an __init_extra__()
method. The problem is that in cases where the base ring is initialised by the category (as it is for Cartesian products), it is also done in an __init_extra__()
method, and in this case the __init_extra__()
of unital algebras is called before the __init_extra__()
of Cartesian products. The order in which the __init_extra__()
methods of the various categories are called is determined by the method resolution order; since this is dynamically constructed by the category framework, I think it will be hard to come up with a robust way of always getting the order correct, so I think it is better to be flexible and have _coerce_map_from_()
as a fallback, allowing us to retry registering the coercion after all __init_extra__()
methods have run.
comment:21 in reply to: ↑ 20 ; follow-up: ↓ 22 Changed 2 years ago by
Replying to pbruin:
Replying to tscrim:
I think your
_lmul_
can create elements in the parent that are not actually suppose to be in the parent. Specifically,_cartesian_product_of_elements
does not do any safety checking (nor do I think it should as it is meant to be called internally with safe input). So in your example, if you instead multiplied by1/5
, I think you get an element ofB
that is not inB
.It doesn't seem this happens, because
_lmul_()
itself is also called with safe input:sage: A = FreeModule(ZZ, 2) sage: B = cartesian_product([A, A]) sage: 1/5*B(([1, 2], [3, 4])) Traceback (most recent call last): ... TypeError: unsupported operand parent(s) for *: 'Rational Field' and 'The Cartesian product of (Ambient free module of rank 2 over the principal ideal domain Integer Ring, Ambient free module of rank 2 over the principal ideal domain Integer Ring)'
I guess I was misremembering, so it is a bit harder to define an action using _lmul_
(and _rmul_
) than _acted_on_
(which does not check beforehand). I thought they had the same behavior in that regards. (Side note, now I am curious where the QQ
action on A
is defined.) Although I would argue there should be an action supported on B
coming from the composition diagonal action of QQ
on A
. I guess that is essentially outside the scope of this ticket.
Also why should we allow for
base_ring is None
in__init_extra__
to be valid? I feel like if we get to that point and there is no base ring, then it is a bug (as we have declared the syntax of the base ring must be setup during the__init__
call) and it should result in an error.The error reported in this ticket occurs inside an
__init_extra__()
method. The problem is that in cases where the base ring is initialised by the category (as it is for Cartesian products), it is also done in an__init_extra__()
method, and in this case the__init_extra__()
of unital algebras is called before the__init_extra__()
of Cartesian products. The order in which the__init_extra__()
methods of the various categories are called is determined by the method resolution order; since this is dynamically constructed by the category framework, I think it will be hard to come up with a robust way of always getting the order correct, so I think it is better to be flexible and have_coerce_map_from_()
as a fallback, allowing us to retry registering the coercion after all__init_extra__()
methods have run.
Ah, I see the issue. The categories of Cartesian products of modules is incomparable (in the poset category) with unital algebras. However, this is an argument for keeping a (cached) method base_ring
that also sets _base
in the category as base_ring()
should be callable and initialized after __init__
has run. The other option that comes to my mind would be to explicitly call the UnitalAlgebras.CartesianProducts.__init_extra__
when base_ring is None
as a way of stating we are forcing the ordering of the categories that should be; we would also make this the first if
statement in that block. What do you think?
comment:22 in reply to: ↑ 21 ; follow-up: ↓ 23 Changed 2 years ago by
Replying to tscrim:
Although I would argue there should be an action supported on
B
coming from the composition diagonal action ofA
. I guess that is essentially outside the scope of this ticket.
Yes, this has more to do with automatically constructing the "right" parent as in push-outs and functorial constructions.
Also why should we allow for
base_ring is None
in__init_extra__
to be valid? I feel like if we get to that point and there is no base ring, then it is a bug (as we have declared the syntax of the base ring must be setup during the__init__
call) and it should result in an error.The error reported in this ticket occurs inside an
__init_extra__()
method. The problem is that in cases where the base ring is initialised by the category (as it is for Cartesian products), it is also done in an__init_extra__()
method, and in this case the__init_extra__()
of unital algebras is called before the__init_extra__()
of Cartesian products. The order in which the__init_extra__()
methods of the various categories are called is determined by the method resolution order; since this is dynamically constructed by the category framework, I think it will be hard to come up with a robust way of always getting the order correct, so I think it is better to be flexible and have_coerce_map_from_()
as a fallback, allowing us to retry registering the coercion after all__init_extra__()
methods have run.Ah, I see the issue. The categories of Cartesian products of modules is incomparable (in the poset category) with unital algebras.
Yes, exactly.
However, this is an argument for keeping a (cached) method
base_ring
that also sets_base
in the category asbase_ring()
should be callable and initialized after__init__
has run.
I don't think there is a problem here; the __init_extra__()
methods are called as the last thing in Parent.__init__()
, so after __init__()
has finished, everything has been set up.
The other option that comes to my mind would be to explicitly call the
UnitalAlgebras.CartesianProducts.__init_extra__
whenbase_ring is None
as a way of stating we are forcing the ordering of the categories that should be; we would also make this the firstif
statement in that block. What do you think?
I don't like this very much; it sounds like an ad-hoc way of enforcing a certain order different from the method resolution order. Also, this would mean that UnitalAlgebras.CartesianProducts.__init_extra__()
would be called twice.
comment:23 in reply to: ↑ 22 ; follow-up: ↓ 24 Changed 2 years ago by
- Milestone changed from sage-8.1 to sage-9.1
- Reviewers set to Travis Scrimshaw
Replying to pbruin:
Replying to tscrim:
However, this is an argument for keeping a (cached) method
base_ring
that also sets_base
in the category asbase_ring()
should be callable and initialized after__init__
has run.I don't think there is a problem here; the
__init_extra__()
methods are called as the last thing inParent.__init__()
, so after__init__()
has finished, everything has been set up.
I think I am convinced now that this is okay. Aince the coercion framework doesn't register a coercion from a parent P
to itself, then there are no differences between defining the coercion between set with __init_extra__
and _coerce_map_from_
. It feels like we should just outright remove the __init_extra__
now since it is functionally useless with the _coerce_map_from_
. Although in order to do this, we have to require that all parents in the category always make a base call of _coerce_map_from_
if they have their own implementation. I think in general this is a good thing to do, but also maybe for a followup as this is a definite bug-fix.
I am not fully convinced that we should go through another super call of _coerce_map_from_
, but since it should be called sparingly due to coercion caching, it shouldn't affect computation times. So if you are definitely okay with the current state, then you can set a positive review. Thank you for answering all my questions and addressing all my concerns.
Side note: A slight inconsistency I noticed
sage: ZZ.coerce_map_from(ZZ) Identity endomorphism of Integer Ring sage: coercion_model.discover_coercion(ZZ, ZZ) (None, None) sage: coercion_model.discover_coercion(ZZ, QQ) ((map internal to coercion system -- copy before use) Natural morphism: From: Integer Ring To: Rational Field, None) sage: QQ.coerce_map_from(ZZ) Natural morphism: From: Integer Ring To: Rational Field
but since discover_coercion
is meant to be more internal-use only, this isn't anything that we need to fix IMO.
comment:24 in reply to: ↑ 23 ; follow-up: ↓ 25 Changed 2 years ago by
- Status changed from needs_review to positive_review
Replying to tscrim:
Replying to pbruin:
Replying to tscrim:
However, this is an argument for keeping a (cached) method
base_ring
that also sets_base
in the category asbase_ring()
should be callable and initialized after__init__
has run.I don't think there is a problem here; the
__init_extra__()
methods are called as the last thing inParent.__init__()
, so after__init__()
has finished, everything has been set up.I think I am convinced now that this is okay. Aince the coercion framework doesn't register a coercion from a parent
P
to itself, then there are no differences between defining the coercion between set with__init_extra__
and_coerce_map_from_
.
Indeed, it shouldn't matter when or how the coercion was registered.
It feels like we should just outright remove the
__init_extra__
now since it is functionally useless with the_coerce_map_from_
. Although in order to do this, we have to require that all parents in the category always make a base call of_coerce_map_from_
if they have their own implementation.
This was actually more or less how the previous branch (from 2017) worked, but from your comments at the time it seemed that you were against this...
I am not fully convinced that we should go through another super call of
_coerce_map_from_
, but since it should be called sparingly due to coercion caching, it shouldn't affect computation times. So if you are definitely okay with the current state, then you can set a positive review. Thank you for answering all my questions and addressing all my concerns.
OK, thank you for the review!
comment:25 in reply to: ↑ 24 Changed 2 years ago by
Replying to pbruin:
Replying to tscrim:
I think I am convinced now that this is okay. Aince the coercion framework doesn't register a coercion from a parent
P
to itself, then there are no differences between defining the coercion between set with__init_extra__
and_coerce_map_from_
.Indeed, it shouldn't matter when or how the coercion was registered.
It feels like we should just outright remove the
__init_extra__
now since it is functionally useless with the_coerce_map_from_
. Although in order to do this, we have to require that all parents in the category always make a base call of_coerce_map_from_
if they have their own implementation.This was actually more or less how the previous branch (from 2017) worked, but from your comments at the time it seemed that you were against this...
I was because I didn't see the necessity of changing the coercion registration mechanism and I wanted it to be done at initialization. I am still a little hesitant about doing it because it feels like it gives a little less control (or at least automatically) with how objects register coercions (e.g., there could be a natural coercion between objects as modules but not as graded modules), but the implementation just has to be more specific with the super()
calls. Thanks again for bearing with me.
comment:26 Changed 2 years ago by
- Branch changed from u/pbruin/19225-cartesian_product_algebra to dcc25eadbd49dd1af066ffc8f76766745bffa361
- Resolution set to fixed
- Status changed from positive_review to closed
comment:27 follow-up: ↓ 30 Changed 22 months ago by
- Commit dcc25eadbd49dd1af066ffc8f76766745bffa361 deleted
Unfortunately I cannot infer from the comments what one has to do if one has used the "ugly _no_generic_basering_coercion hack" in the past.
Shall one simply remove that attribute? Or what else to implement?
It is really unfortunate that clicking on the branch of a closed ticket doesn't provide information on what the ticket changed (or at least clicking on the branch for THIS ticket and searching for the string "_no_generic_basering_coercion" doesn't show a result).
comment:28 follow-up: ↓ 29 Changed 22 months ago by
Of course I didn't empty the "commit" field on purpose.
comment:29 in reply to: ↑ 28 Changed 22 months ago by
Replying to SimonKing:
Of course I didn't empty the "commit" field on purpose.
... and when I tried to fill it with its old value, it didn't help. I infer that the commit field of a closed ticket is of no use.
comment:30 in reply to: ↑ 27 ; follow-up: ↓ 31 Changed 22 months ago by
Replying to SimonKing:
Unfortunately I cannot infer from the comments what one has to do if one has used the "ugly _no_generic_basering_coercion hack" in the past.
Shall one simply remove that attribute? Or what else to implement?
You can try removing it. If that doesn't work, you can implement a _coerce_map_from_base_ring()
method; see the branches attached to this ticket and to #29247 for some examples. I think it is possible to make _coerce_map_from_base_ring()
return None
in case you really don't want a coercion map.
It is really unfortunate that clicking on the branch of a closed ticket doesn't provide information on what the ticket changed (or at least clicking on the branch for THIS ticket and searching for the string "_no_generic_basering_coercion" doesn't show a result).
In this case, that happens because _no_generic_basering_coercion
was kept in place in this ticket and was only removed in #29247.
comment:31 in reply to: ↑ 30 Changed 22 months ago by
Replying to pbruin:
Shall one simply remove that attribute? Or what else to implement?
You can try removing it. If that doesn't work, you can implement a
_coerce_map_from_base_ring()
method; see the branches attached to this ticket and to #29247 for some examples. I think it is possible to make_coerce_map_from_base_ring()
returnNone
in case you really don't want a coercion map.
Thank you!
As it turns out, removing the attribute did work in my application.
solves this bug, but then