Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KeyBuilder: change int to direct str representation #261

Closed
wants to merge 7 commits into from
Closed

KeyBuilder: change int to direct str representation #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7546ab8
KeyBuilder: change int to direct str representation
matthiasdiener Sep 3, 2024
c93f325
clarify relations between numpy/python scalars
matthiasdiener Oct 4, 2024
20fe54e
remove now incorrect note
matthiasdiener Oct 4, 2024
69d6aab
Merge branch 'main' into keyb-int-str
matthiasdiener Nov 5, 2024
790dddd
Merge branch 'main' into keyb-int-str
matthiasdiener Nov 22, 2024
26e0492
Merge branch 'main' into keyb-int-str
matthiasdiener Jan 16, 2025
bda35f3
Merge branch 'main' into keyb-int-str
matthiasdiener Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 1 addition & 16 deletions pytools/persistent_dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,6 @@ class KeyBuilder:
may stop working as early as 2022.

.. versionadded:: 2021.2

.. note::

Some key-building uses system byte order, so the built keys may not match
across different systems. It would be desirable to fix this, but this is
not yet done.
"""

# this exists so that we can (conceivably) switch algorithms at some point
Expand Down Expand Up @@ -274,16 +268,7 @@ def update_for_type(self, key_hash: Hash, key: type) -> None:
update_for_ABCMeta = update_for_type

def update_for_int(self, key_hash: Hash, key: int) -> None:
sz = 8
while True:
try:
# Must match system byte order so that numpy and this
# generate the same string of bytes.
# https://github.com/inducer/pytools/issues/259
key_hash.update(key.to_bytes(sz, byteorder=sys.byteorder, signed=True))
return
except OverflowError:
sz *= 2
key_hash.update(str(key).encode("utf-8"))

def update_for_enum(self, key_hash: Hash, key: Enum) -> None:
self.update_for_str(key_hash, str(key))
Expand Down
13 changes: 10 additions & 3 deletions pytools/test/test_persistent_dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,9 +461,16 @@ def test_scalar_hashing() -> None:
assert keyb(np.int32(1)) == keyb(np.int32(1))
assert keyb(np.int32(2)) != keyb(np.int32(1))
assert keyb(np.int64(1)) == keyb(np.int64(1))
assert keyb(1) == keyb(np.int64(1))
assert keyb(1) != keyb(np.int64(1))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I thought the goal was to keep them the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From your answer to my question in #260, my interpretation was that they don't need to be the same. Changing the code like in this PR perhaps also addresses #262.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They need to be the same. The fundamental property of a hash function $h$ is that $x = y \Rightarrow h(x) = h(y)$. Since

>>> np.int32(1) == 1
True

our hands are tied. I'm sorry if my early comment was ambiguous.

Copy link
Contributor Author

@matthiasdiener matthiasdiener Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we already break this principle in some (many?) places:

>>> from pytools.persistent_dict import KeyBuilder
>>> keyb = KeyBuilder()
>>> 1.0 == 1
True
>>> keyb(1.0) == keyb(1)
False

(with current main branch)

assert keyb(1) != keyb(np.int32(1))

assert keyb(1) == keyb("1")
assert keyb(1.0) != keyb("1.0") # '1.0' uses hex representation
assert keyb(1+1j) == keyb("(1+1j)")
Comment on lines +467 to +469
Copy link
Contributor Author

@matthiasdiener matthiasdiener Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is an issue. If it is, we could change the KeyBuilder to (int, str(key)).


assert keyb(np.float64(1.0)) != keyb(1.0)
assert keyb(np.complex128(1.0+2.0j)) != keyb(1.0+2.0j)

assert keyb(np.longlong(1)) == keyb(np.longlong(1))

assert keyb(np.float16(1.1)) == keyb(np.float16(1.1))
Expand Down Expand Up @@ -595,7 +602,7 @@ class MyDC:
name: str
value: int

assert keyb(MyDC("hi", 1)) == "d1a1079f1c10aa4f"
assert keyb(MyDC("hi", 1)) == "72c25f5d0ac2bda7"

assert keyb(MyDC("hi", 1)) == keyb(MyDC("hi", 1))
assert keyb(MyDC("hi", 1)) != keyb(MyDC("hi", 2))
Expand All @@ -619,7 +626,7 @@ class MyAttrs:
name: str
value: int

assert (keyb(MyAttrs("hi", 1)) == "5b6c5da60eb2bd0f") # type: ignore[call-arg]
assert (keyb(MyAttrs("hi", 1)) == "6e8bfc4394f82985") # type: ignore[call-arg]

assert keyb(MyAttrs("hi", 1)) == keyb(MyAttrs("hi", 1)) # type: ignore[call-arg]
assert keyb(MyAttrs("hi", 1)) != keyb(MyAttrs("hi", 2)) # type: ignore[call-arg]
Expand Down
Loading