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 bug in hashing samples #151

Merged
merged 2 commits into from
Jan 26, 2024
Merged

fix bug in hashing samples #151

merged 2 commits into from
Jan 26, 2024

Conversation

ericphanson
Copy link
Member

@ericphanson ericphanson commented Jan 25, 2024

The test I added fails on main:

julia> @test hash(samples) == hash(samples2)
Test Failed at REPL[23]:1
  Expression: hash(samples) == hash(samples2)
   Evaluated: 0x9bab1974999310ac == 0x877a3e1403901e84

but passes on this PR.

closes #150

Copy link
Member

@palday palday left a comment

Choose a reason for hiding this comment

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

I think this makes sense, but I think @ararslan should weigh in on whether this will mess with anything in Julia internals relying on Base.hash

Copy link
Member

@kimlaberinto kimlaberinto left a comment

Choose a reason for hiding this comment

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

Looks good! And seems to make sense as it follows what is recommended in Base.hash's docstring to implement the 2-argument form and calling it recursively to to mix the hashes of the contents with each other.

(docstring from Julia 1.9.4)

  hash(x[, h::UInt]) -> UInt

  Compute an integer hash code such that isequal(x,y) implies hash(x)==hash(y). The optional second argument
  h is another hash code to be mixed with the result.

  New types should implement the 2-argument form, typically by calling the 2-argument hash method recursively
  in order to mix hashes of the contents with each other (and with h). Typically, any type that implements
  hash should also implement its own == (hence isequal) to guarantee the property mentioned above. Types
  supporting subtraction (operator -) should also implement widen, which is required to hash values inside
  heterogeneous arrays.

@ericphanson
Copy link
Member Author

The CI failure looks unrelated but concerning

@palday
Copy link
Member

palday commented Jan 25, 2024

I can replicate the CI failure on a local run and it also happens on main. Did an update in Arrow break something?

@palday
Copy link
Member

palday commented Jan 25, 2024

Tests pass on main with Arrow set to 2.6 -- current Arrow release is 2.7.

@palday
Copy link
Member

palday commented Jan 25, 2024

see #153.

This is definitely something that needs to be fixed in upstream Arrow, but we have a work around that's trivial

@ericphanson
Copy link
Member Author

This is definitely something that needs to be fixed in upstream Arrow, but we have a work around that's trivial

Just to say, I don't think there was any bug in the Arrow release, but rather in Onda we were depending on Legolas generating a specific fromarrow method that we stopped generating after beacon-biosignals/Legolas.jl#106, since Samples has some custom serialization as it is not a Legolas @version. Resolved by #153 in any case.

@palday palday merged commit 4664841 into main Jan 26, 2024
8 checks passed
@palday palday deleted the eph/hash branch January 26, 2024 18:10
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.

Missing method for Base.hash
4 participants