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

Fix quadrature rule hash #132

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

connorjward
Copy link

@connorjward connorjward commented Jan 31, 2025

Also remove some uses of deprecated @abstractproperty and gives cells and point sets repr()s.

Necessary for firedrakeproject/firedrake#3989

Also remove some uses of deprecated `@abstractproperty` and gives cells
and point sets `repr()`s.
@connorjward
Copy link
Author

@ksagiyam please can you take a look at this too?

Copy link

@ksagiyam ksagiyam left a comment

Choose a reason for hiding this comment

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

This PR itself looks almost fine to me, but floats being used in some __repr__() could cause issues in computing cache key in firedrake.tsfc_interface.

Comment on lines 67 to 68
def __eq__(self, other):
return type(other) is type(self) and hash(other) == hash(self)
Copy link

Choose a reason for hiding this comment

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

I do not think hash() necessarily does perfect hashing, so this definition could in theory go wrong if there was a hash collision. I think we should define __eq__() in child classes comparing sting-by-string, float-by-float, etc.

Copy link
Author

Choose a reason for hiding this comment

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

I think hash() here is safe as we define __hash__() above. What I am doing is equivalent to what happens in finat.ufl.

If we made child classes overload __eq__() then we have some annoying details to handle (source):

If a class that overrides __eq__() needs to retain the implementation of __hash__() from a parent class, the interpreter must be told this explicitly by setting __hash__ = <ParentClass>.__hash__.

I don't know exactly what the standard thing to do here is but this at least works.

Copy link

Choose a reason for hiding this comment

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

As far as I understand, hash() is not something that guarantees equality. If we want to store fruits (say "orange", "apple", "banana"), the simplest hash keys would be len(name), and the hash table looks like:

0:
1:
2:
3:
4:
5: apple
6: orange -> banana

but orange is not banana. This is called hash collision, and this happens unless hashing is perfect. You might not have seen failures just because the __hash__() method you use is "almost" perfect.
orange and banana can be correctly distinguished by implementing __eq__() correctly.

This makes me think about our having been using md5 to generate cache keys, but maybe I misunderstand something about md5.

Copy link

Choose a reason for hiding this comment

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

I have to focus on my own work the rest of the day, so will come back next week.

Copy link
Author

Choose a reason for hiding this comment

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

I changed it to

def __eq__(self, other):
        return type(other) is type(self) and repr(other) == repr(self)

anyway which might be better and avoids any confusion.

@connorjward
Copy link
Author

but floats being used in some repr() could cause issues in computing cache key in firedrake.tsfc_interface.

An excellent catch. I will think on that.

@connorjward
Copy link
Author

This PR itself looks almost fine to me, but floats being used in some repr() could cause issues in computing cache key in firedrake.tsfc_interface.

I have now had a stab at fixing this. UFL provides a format_float function that it uses for this purpose in constantvalue.py.

gem/utils.py Outdated Show resolved Hide resolved
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.

3 participants