Support EDF.jl 0.8 #104
Support EDF.jl 0.8 #104
Conversation
There was a problem hiding this comment.
also needs a change in the export code, here:
Line 26 in aa3eca7
OndaEDFSchemas.jl/Project.toml
Outdated
| @@ -1,7 +1,7 @@ | |||
| name = "OndaEDFSchemas" | |||
| uuid = "9c87d999-769b-4741-85b2-6f554d09e731" | |||
| authors = ["Beacon Biosignals, Inc."] | |||
| version = "0.2.2" | |||
| version = "0.3.0" | |||
There was a problem hiding this comment.
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.
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.
Oh I see, I somehow missed that this is for OndaEDFSchemas.
|
Alright, I did another search to make sure I didn't miss any other places where the type needs to be widened from |
| 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))' |
There was a problem hiding this comment.
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.
@palday did you mean to push this change here?
There was a problem hiding this comment.
tests were failing on 1.6 because whatever Onda version gets resolved there ignored validate=false when using the old 0d array. Sparse matrices avoid that and the allocation of a massive dense array.
There was a problem hiding this comment.
we can drop 1.6 support IMO, it's not longer LTS
There was a problem hiding this comment.
I'm also fine with that, whether or not you leave the sparse change in.
There was a problem hiding this comment.
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.
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 min):
There was a problem hiding this comment.
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.
I'm so sorry. I really didn't mean to bring this upon you!
This upgrades OndaEDF.jl to EDF.jl 0.8 (c.f. beacon-biosignals/EDF.jl#92).
CI won't pass yet, since I'll need to merge beacon-biosignals/EDF.jl#92 and register the package first.