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

Simpler and more canonical hash #55

Closed
wants to merge 1 commit into from

Conversation

eric-wieser
Copy link
Contributor

Marginally faster too

@dirk-thomas
Copy link
Member

This will change the hash of e.g. stored TVal objects. Since this would break stored data which rely on the old hash semantic I don't think this should be changed without a good reason.

@eric-wieser
Copy link
Contributor Author

eric-wieser commented Jun 26, 2016

I'd argue that the only hash semantic is that x == y implies hash(x) == hash(y) - from the docs:

The only required property is that objects which compare equal have the same hash value

But the current implementation fails to meet even that (under a contrived scenario):

dt1 = Duration.from_sec(1.0)
dt1.secs = 2.0  # ok, these properties aren't supposed to be edited...

dt2 = Duration.from_sec(2.0)

assert dt1 == dt2   # ok
assert hash(dt1) == hash(dt2) # AssertionError

Can you elaborate on what you mean about stored data?

In python >3,3, hash() of strings returns a different value each time the python process runs - so the current implementation actually causes a greater problem for people who foolishly rely on a hash implementation.

@dirk-thomas
Copy link
Member

The example indicates that setting secs to a float shouldn't be allowed. Instead all assignments should be converted to integer values. Then the semantic of the current hash function would be perfectly valid.

I thought about cases where users e.g. pickle a dictionary with times as the key. But I guess this only affects the in-memory representation of the dict. Not the pickled data. @ros/ros_team Do you think it is safe to change the hash function here?

Since even the new proposed hash would fail for the above example should we modify the access to the slots and always convert them to int to ensure a normalized representation?

@eric-wieser
Copy link
Contributor Author

Since even the new proposed hash would fail for the above example

Not true, because in python 1 == 1.0, so the hash is required to be equal

@dirk-thomas
Copy link
Member

@eric-wieser good point, that makes quite a difference.

@dirk-thomas
Copy link
Member

Since nobody else raised any concerns I moved forward and cherry-picked your patch to the kinetic-devel branch: b244f2f

Thank you again.

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

Successfully merging this pull request may close these issues.

2 participants