#20246: Use `with strict_equality(True)` to work around hashing of p-adics
-------------------------------------+-------------------------------------
Reporter: saraedum | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-7.2
Component: padics | Resolution:
Keywords: days71 | Merged in:
Authors: Julian Rüth | Reviewers:
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/saraedum/use__with_strict_equality_true___to_work_around_hashing_of_p_adics|
f4c5a9db729ccf911517039fdced282588210133
Dependencies: #16342, #16339 | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by saraedum):
Replying to [comment:23 nbruin]:
> Replying to [comment:22 roed]:
> > The huge problem with `cache_key` is that any object that can
recursively contain an inexact element needs to now define a `cache_key`.
Polynomials, matrices, parents that are defined by polynomials.... It's a
nightmare to add all of these `cache_keys`.
>
> Only the ones that actually get used as cache keys need one.
However, most parents get used as cache keys in our factories and
`UniqueRepresentation`. And parents may depend on p-adic numbers in some
way.
> Using a p-adic number as a dictionary key (including a cache) is likely
a programming/logic error, so the only places where you need to allow it
is where you think the convenience of being able to do it is sufficiently
important.
I agree that it is usually a programming error. Therefore, you want to
make sure that people explicitly select the right notion of equality to
use them. Note that this is also a problem for fraction field elements.
> There's no need to universally announce that any object containing
p-adics can be used as a cache key.
No but a lot of our code assumes that everything is usable as a key in a
dictionary for the sake of caching. I believe that this is a reasonable
assumption: Running a method on indistinguishable inputs should produce
the same output. And in some cases it should even produce the exact same
object, for example when creating parents; in those cases you can not do
without caching.
> And if I understand correctly, the cache_key can be implemented quite
high in the hierarchy, so you wouldn't need to so terribly many of them,
and their implementations are trivial.
I am not sure what you mean with "high up". For correctness you have to
implement it in any class that gets instantiated with a parameter that may
depend (in some way) on something unhashable, because essentially you just
need to write this as a method that produces the constructor parameters
that serve to produce an indistinguishable element.
> I can see how it might seem like a nightmare, but actually doing it for
the cases where you need it might not be so bad. The remaining cases will
get pretty good error messages because it's just a hashing failure.
Though I originally implemented `_cache_key` what I dislike about it is
that you just have to add silly boilerplate to many classes. The new
approach is much better in that sense. The changes are where the actual
problem arises, namely in the classes that can not define a usual
`__hash__`. For everybody else it just works and it is actually correct.
The current `_cache_key` is calling for trouble in the long run: People
add a constructor parameter but don't add it to `_cache_key`; the doctests
pass but caching eventually produces incorrect results.
Of course, if the end user decides to run all its code `with
strict_equality(True)` then many things break. But the docstring tries to
be clear about this.
> > And if `strict_equality` is used sparingly, I don't think it's that
bad.
> I think that qualification already relegates it to a last resort
solution, only to be implemented if the others are thoroughly unworkable.
And implementing `cache_key` routines in strategic places as the need
arises should be quite workable.
I think the "strategic places" is a problem here. What this says it that
using p-adics (or fraction field elements) just works in places where
there happened to be a doctest that implied p-adics (and usually extension
fields to actually trigger `__hash__`.) Why not make it work everywhere?
> I really think making the meaning of equality tests depend on the
program state is a very undesirable step to make and compared to that,
implementing some `cache_key` routines doesn't seem so bad.
Yes, global state is bad. The implementation is very clear on that and I
can elaborate on this further in the docstrings. If you enable strict
equality and then call into other code you are calling for trouble. That
is why you only enable it very locally and can even disable it again more
locally if you need to call out.
--
Ticket URL: <http://trac.sagemath.org/ticket/20246#comment:24>
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 https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
