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.

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

persistent_dict can't store NaNs #287

Open
matthiasdiener opened this issue Feb 5, 2025 · 3 comments
Open

persistent_dict can't store NaNs #287

matthiasdiener opened this issue Feb 5, 2025 · 3 comments

Comments

@matthiasdiener
Copy link
Contributor

from pytools.persistent_dict import PersistentDict
import numpy as np

pdict = PersistentDict("pytools-test", safe_sync=True)

pdict[np.nan] = 42
assert pdict[np.nan] == 42
$ python t.py
/Users/mdiener/Work/e12test/pytools/pytools/persistent_dict.py:602: CollisionWarning: pytools-test: key collision in cache at '/Users/mdiener/Work/e12test/loopy/xdg-cache/pytools' -- these are sufficiently unlikely that they're often indicative of a broken hash key implementation (that is not considering some elements relevant for equality comparison)
  return self.fetch(key)
Traceback (most recent call last):
  File "/Users/mdiener/Work/e12test/pytools/t.py", line 8, in <module>
    assert pdict[np.nan] == 42
           ~~~~~^^^^^^^^
  File "/Users/mdiener/Work/e12test/pytools/pytools/persistent_dict.py", line 602, in __getitem__
    return self.fetch(key)
           ^^^^^^^^^^^^^^^
  File "/Users/mdiener/Work/e12test/pytools/pytools/persistent_dict.py", line 840, in fetch
    self._collision_check(key, stored_key)
  File "/Users/mdiener/Work/e12test/pytools/pytools/persistent_dict.py", line 552, in _collision_check
    raise NoSuchEntryCollisionError(key)
pytools.persistent_dict.NoSuchEntryCollisionError: nan

🤦

Perhaps we should also check if the value is a Nan in the collision check? Are there other cases that could be at issue here (inf seems to be fine)?

cc inducer/loopy#828 [major plot twist!]

@inducer
Copy link
Owner

inducer commented Feb 5, 2025

We can't really check for NaN in persistent dict, as we would have to traverse the entire key (expensive!). We'll need to rely on the user to not have NaN values. It might be worth a word of warning in the persistent dict documentation though.

Beyond that, I think this is more a loopy bug. It should replace NaN values with pymbolic NaNs.

@matthiasdiener
Copy link
Contributor Author

matthiasdiener commented Feb 5, 2025

Could we emit a warning while generating the key when we encounter a NaN? (done in #288)

Edit: Or perhaps, set a flag somewhere for the key that stores whether key contained a NaN, and perform the key traversal when that flag is set in the collision check.

@inducer
Copy link
Owner

inducer commented Feb 5, 2025

"Walking a value" is not really well-defined in Python, so I'd rather not try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants