Add a CommTag class with a stable hash #319
Conversation
matthiasdiener
force-pushed
the
commtag
branch
from
f057714
to
f251f71
Compare
Co-authored-by: Matt Smith <[email protected]>
matthiasdiener
force-pushed
the
commtag
branch
from
8980476
to
35183b2
Compare
grudge/trace_pair.py
Outdated
| def interior_trace_pairs(dcoll: DiscretizationCollection, vec, *, | ||
| comm_tag: Hashable = None, tag: Hashable = None, | ||
| comm_tag: CommTag = None, tag: Hashable = None, |
There was a problem hiding this comment.
I don't agree with this change. There's no good reason why other Hashables should not be accepted.
There was a problem hiding this comment.
I undid this change in 9b2ced4. I left the warning in https://github.com/inducer/grudge/pull/319/files#diff-5fa6e6b713d37028c16f1bbc5a6d0b4547ecba0f86c8a52eb30e1ba1dc2614e4R518 in place for now, would you like me to remove it?
Edit:
Removed in 9454067
| @@ -318,8 +319,20 @@ def interior_trace_pair(dcoll: DiscretizationCollection, vec) -> TracePair: | |||
| return local_interior_trace_pair(dcoll, vec) | |||
|
|
|||
|
|
|||
| @dataclass(frozen=True) # for KeyBuilder support | |||
| class CommTag: | |||
There was a problem hiding this comment.
It doesn't really make sense for all CommTags to be subclasses of this. Some may have data contained in them.
There was a problem hiding this comment.
I think we should stick to arbitrary-data-with-certain-requirements as tags. I mostly don't want to restrict to just subclasses of something specific.
There was a problem hiding this comment.
Ok, would you like to keep the CommTag class as an optional way to construct the comm tags? I removed the warning in 9454067.
grudge/trace_pair.py
Outdated
| return hash(tuple(str(type(self)).encode("ascii"))) | ||
|
|
||
| def __eq__(self, other): | ||
| return isinstance(other, type(self)) |
There was a problem hiding this comment.
This is incorrect: It accepts arbitrary subclasses for other, and violates the property of "__eq__ implies hash equality."
There was a problem hiding this comment.
This is (hopefully) fixed in 6918c63
matthiasdiener
force-pushed
the
commtag
branch
from
5e21cf2
to
9a63df4
Compare
- don't use dataclass anymore, implement update_persistent_hash - test _sym_tag_to_num_tag - (hopefully) fix __eq__
matthiasdiener
force-pushed
the
commtag
branch
from
2b6630d
to
6918c63
Compare
matthiasdiener
force-pushed
the
commtag
branch
from
62e68b2
to
e7f9239
Compare
|
It's unlikely this will be needed for deterministic execution, closing. |
TODO: