Skip to content

Remove DagNode.__hash__()#5979

Merged
mergify[bot] merged 2 commits into
Qiskit:masterfrom
levbishop:remove_hash
Mar 5, 2021
Merged

Remove DagNode.__hash__()#5979
mergify[bot] merged 2 commits into
Qiskit:masterfrom
levbishop:remove_hash

Conversation

@levbishop
Copy link
Copy Markdown
Member

Defining a __hash__ without also defining __eq__ goes against the python data model.

It looks like in the past there was a custom __eq__ but that was removed in #2006 at which time __hash__ should have been removed also.

Its also not necessary in this case as a hash based on the object id is what you get by default.

Its also a performance issue: microbenchmarking reveals that set and dict operations are around 10x slower with custom hash compared to default. I didn't check but I wonder how much this was at issue in #5952 and #5685.

@levbishop levbishop added bug Something isn't working performance labels Mar 5, 2021
@levbishop levbishop requested a review from a team as a code owner March 5, 2021 18:20
@kdk kdk added the automerge label Mar 5, 2021
@kdk kdk added this to the 0.17 milestone Mar 5, 2021
@mtreinish
Copy link
Copy Markdown
Member

mtreinish commented Mar 5, 2021

Its also a performance issue: microbenchmarking reveals that set and dict operations are around 10x slower with custom hash compared to default. I didn't check but I wonder how much this was at issue in #5952 and #5685.

Well at least for #5952 this won't come up, the retworkx neighbors() function is returning node ids (https://retworkx.readthedocs.io/en/stable/stubs/retworkx.PyDiGraph.html#retworkx.PyDiGraph.neighbors) so it's all ints (also the performance impact of that PR was minimal https://qiskit.github.io/qiskit/#mapping_passes.PassBenchmarks.time_noise_adaptive_layout?commits=5f0fdb6 )

I could definitely see this being a big part of the impact of #5685 though.

@mergify mergify Bot merged commit 8f45a79 into Qiskit:master Mar 5, 2021
@levbishop levbishop deleted the remove_hash branch March 9, 2021 15:03
@kdk kdk added Changelog: None Do not include in the GitHub Release changelog. Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. labels Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. Changelog: None Do not include in the GitHub Release changelog. performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants