the-siesta-group / edfio

Read and write EDF/EDF+ files.
Apache License 2.0
28 stars 4 forks source link

Support missing subfields in EDF+ header fields. #18

Closed marcoross closed 9 months ago

marcoross commented 10 months ago

Increase tolerance regarding EDF+ files not compliant with the EDF+ standard. At the same time, make the API more comfortable to use by accepting None or empty strings as unspecified subfields.

hofaflo commented 10 months ago

Great idea! I'd like to avoid return type unions with None wherever possible, as this makes the type annotations much less helpful. Can we instead return "X" for all non-existent fields? This would also allow to keep the __init__ methods as they are.

cbrnr commented 10 months ago

The first field of local_recording_information must always be Startdate, so I'd use this as a default instead of X.

hofaflo commented 10 months ago

The first field of local_recording_information must always be Startdate, so I'd use this as a default instead of X.

Accessing this field via edfio.Recording is currently not possible. Do you think it should be?

cbrnr commented 10 months ago

Accessing this field via edfio.Recording is currently not possible. Do you think it should be?

I saw it was printed in the repr though. This question is orthogonal to what the field's default value should be, right? I guess it should be accessible somewhere, not sure what the best place is.

marcoross commented 9 months ago

Great idea! I'd like to avoid return type unions with None wherever possible, as this makes the type annotations much less helpful. Can we instead return "X" for all non-existent fields? This would also allow to keep the __init__ methods as they are.

I changed the return types to str and return "X" in case of unspecified subfields.

marcoross commented 9 months ago

Accessing this field via edfio.Recording is currently not possible. Do you think it should be?

I saw it was printed in the repr though. This question is orthogonal to what the field's default value should be, right? I guess it should be accessible somewhere, not sure what the best place is.

I made the get_subfield(idx: int) method public, so now it can be accessed using this method with index 0.

hofaflo commented 9 months ago

I'd prefer to avoid allowing to pass None instead of "X" (or omitting the parameter) when instantiating a Patient or Recording. This would simplify the signatures and let us avoid some is None checks. Do you see a use case that requires to accept None?

marcoross commented 9 months ago

I made the requested change. I don't know, however, if this is still relevant given the ongoing discussion about strictness regarding the EDF+ standard and the possibility of having an "auto-fix" method.

hofaflo commented 9 months ago

I don't know, however, if this is still relevant given the ongoing discussion about strictness regarding the EDF+ standard and the possibility of having an "auto-fix" method.

I think it still makes sense to have this, as one might want to access header fields without trying to autofix stuff.

Note: I rebased and force-pushed to fix the position of the changelog entry.

marcoross commented 9 months ago

I made all requested changes in the last commit.