-
Notifications
You must be signed in to change notification settings - Fork 2
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
Support EDF.jl 0.8 #104
Support EDF.jl 0.8 #104
Changes from 5 commits
4d40576
279a8c6
3990aea
8204919
df15560
2e7ba31
8eaa8de
6501c3b
a4054a7
187e66c
051d096
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,9 @@ | |
@testset "Record metadata" begin | ||
function change_sample_rate(samples; sample_rate) | ||
info = SamplesInfoV2(Tables.rowmerge(samples.info; sample_rate=sample_rate)) | ||
new_data = similar(samples.data, 0, Onda.index_from_time(sample_rate, Onda.duration(samples)) - 1) | ||
# sparse CSC but for wide data we have a huge column pointer | ||
# so transposing to get sparse CSR | ||
new_data = spzeros(eltype(samples.data), Onda.index_from_time(sample_rate, Onda.duration(samples)) - 1, channel_count(samples))' | ||
Comment on lines
-34
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this change; AFAICT it's not really related to the EDF.jl changes, but maybe I'm missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @palday did you mean to push this change here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tests were failing on 1.6 because whatever Onda version gets resolved there ignored There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can drop 1.6 support IMO, it's not longer LTS There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also fine with that, whether or not you leave the sparse change in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To keep this PR focused I'm inclined to drop the change as @kleinschmidt suggests and bump support to 1.10. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. strangely that does not seem to have done it....still getting the same validation error on 1.10. AFAICT the same version of Onda is used in both CI runs (1 and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turns out this is still a problem in 1.10, so I'm going to keep @palday's change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm so sorry. I really didn't mean to bring this upon you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😆 |
||
return Samples(new_data, info, samples.encoded; validate=false) | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is breaking since it's just adding new schemas that use the widened field type rather than changing any existing schemas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now this only supports EDF 0.8 (not 0.7 and earlier). This means that the returned value of a number of functions is FilePlanV3 / PlanV3 instead of V2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, I somehow missed that this is for OndaEDFSchemas.