Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Simpler and more canonical hash#55

Closed
eric-wieser wants to merge 1 commit intoros:indigo-develfrom
eric-wieser:patch-2
Closed

Simpler and more canonical hash#55
eric-wieser wants to merge 1 commit intoros:indigo-develfrom
eric-wieser:patch-2

Conversation

@eric-wieser
Copy link
Copy Markdown
Contributor

Marginally faster too

@dirk-thomas
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@dirk-thomas
Copy link
Copy Markdown
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants