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

LoopyKeyBuilder: improve BasicSet handling #913

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

matthiasdiener
Copy link
Collaborator

@matthiasdiener matthiasdiener commented Feb 6, 2025

This is (very) crude, not sure if a viable direction.

Fixes #912.

TODO:

  • check Map

@matthiasdiener matthiasdiener self-assigned this Feb 6, 2025
Comment on lines +358 to +366
# Equality
assert a == b
assert a.is_equal(b)
assert not a.plain_is_equal(b)

# Hashing
assert hash(a) != hash(b)
assert a.get_hash() != b.get_hash()
assert LoopyKeyBuilder()(a) == LoopyKeyBuilder()(b)
Copy link
Owner

Choose a reason for hiding this comment

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

This behavior is pretty 💩-tastic. I'm not super interested in codifying it with a test.

If I could snap my finger and have a solution enacted globally, probably I would like ==, plain_is_equal and __hash__ to all share a notion of equality. Something for namedisl to aspire to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aside from the test, would you still be interested in changing the way the hash is calculated, as in this PR?

Copy link
Owner

Choose a reason for hiding this comment

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

I think namedisl is the only reasonable path forward. I don't know that I'd like to mess with the status quo too much until that time, unless there is a pressing reason. Is there one, in the context of this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think there is a pressing need for this PR. The goal was to enable running the tests with LOOPY_ABORT_ON_CACHE_MISS (#828).

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

Successfully merging this pull request may close these issues.

LoopyKeyBuilder: hash for BasicSet differs for objects that compare equal
2 participants