Skip to content

Conversation

@kylebd99
Copy link
Collaborator

I'm just a bit paranoid about the existence of a hash bug in OrderedSet, so for the sake of prudence I suggest we use SortedSet. My guess is that the performance impact will be fairly negligible.

@willow-ahrens
Copy link
Member

willow-ahrens commented Apr 18, 2025

does this work? I tried this but realized we needed to isless the keys, which is a little fraught because many things in Finch do not have a total order. We could use StableHash, but it keeps references to everything so the whole code would need to be modified to accept a limited-lifetime hash function object.

@willow-ahrens
Copy link
Member

You're right that this is a problem though, because galley tests now fail on the finch-tensor-python. A fix is proposed in JuliaCollections/OrderedCollections.jl#149, which we could pull in for now?

@kylebd99
Copy link
Collaborator Author

kylebd99 commented Apr 18, 2025

That works for me. Let's just pull in the fix. I'll add it to this branch.

@willow-ahrens
Copy link
Member

I've got an alternate approach to just use Set when it's the key of a dictionary: #736

@codecov
Copy link

codecov bot commented Apr 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

2 participants