#19016: Better hash for Element
-------------------------------------+-------------------------------------
Reporter: ncohen | Owner:
Type: defect | Status: needs_work
Priority: critical | Milestone: sage-6.9
Component: misc | Resolution:
Keywords: | Merged in:
Authors: Nils Bruin, | Reviewers:
Vincent Delecroix | Work issues:
Report Upstream: N/A | Commit:
Branch: public/19016 | a0f830bf8dc0cbdeb75e45ee7bfee7b4fad33768
Dependencies: | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by nbruin):
Found it: the doctest is indeed broken.
Replying to [comment:79 nbruin]:
> {{{
> sage: K.<a> = NumberField(x^2 + 2*3*7*11)
> sage: M = parent(a.matrix())
> sage: K_into_MS = K.hom([a.matrix()])
> sage: K._unset_coercions_used()
> sage: K.register_embedding(K_into_MS)
> sage: M.get_action(K) is None
> True
> }}}
The appropriate way to construct the field would be via
{{{
sage: Ktemp.<atemp> = NumberField(x^2 + 2*3*7*11)
sage: K.<a> = NumberField(atemp.minpoly(),embedding=atemp.matrix())
TypeError: <type
'sage.matrix.matrix_rational_dense.Matrix_rational_dense'> is not hashable
and does not implement _cache_key()
}}}
which shows the real source of the change in behaviour: previously, the
matrix used to define the embedding couldn't be used as a key in the
`UniqueFactory` cache. On our very first commit, I introduce a
`_cache_key` to compensate for the missing hash. So unhashable matrices
can now be used as cache keys (but of course they shouldn't).
'''Certificate:''' You can toggle the behaviour by activating/deactivating
the definition of _cache_key on Element.
So the doctest used to create a field that couldn't be directly
constructed. With the introduced _cache_key, the field can be constructed.
During action discovery, the field does get reconstructed. This failed in
an unexpected way before, which led to abandoning action discovery and
then to using the specified embedding.
In our new setup, registering the embedding after the fact does not lead
to breaking action discovery anymore, and hence we never get to using the
embedding.
It is documented in `get_coercion_model().bin_op` that it first tries to
find an action and then tries to find a coercion. So this doctest is
relying on erroneous behaviour: There is an action between M and K, so the
meaning of M.0 * K.0 means: let K.0 act on M.0, not "try to coerce K.0
into M and then multiply M.0 with the result".
'''MAIN ACTION POINT:''': kill offending doctest.
Followup work to be done:
- do we want `_cache_key` on elements? For matrices, the implementation
proposed here is robust (i.e., the key is basically what one could get
from making an immutable copy of the thing).
- should embeddings take precedence over actions? I think that this would
mean a horrible overhaul of the coercion framework, so I think the answer
needs to be "no". People need to get used to the fact that implicit
coercion can't do everything for you. Most of the time you're better of
applying maps explicitly.
--
Ticket URL: <http://trac.sagemath.org/ticket/19016#comment:84>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
