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

MRG, BUG: Fix EDF header parsing #8558

Merged
merged 2 commits into from Nov 24, 2020
Merged

MRG, BUG: Fix EDF header parsing #8558

merged 2 commits into from Nov 24, 2020

Conversation

larsoner
Copy link
Member

  1. Implements a variant of @a-hurst's code from EDF/EDF+ headers parsed incompletely/incorrectly #8544 (most of the PR)
  2. Fixes dipy decorator (1 line)
  3. Fixes str-as-int parsing from gitter (1 line)
  4. Adds stub test with XXX for carrying over edf_info['subject_info'] to raw.info['subject_info'], which we should probably do to the extent possible (but in another PR)

@a-hurst the one thing I didn't follow was (what is keeping this WIP from my end):

My only other change was adding recording_info=meas_id, to the edf_info.update() call further down so the recording info gets carried over to the final Raw object.

Can you take a look, and clarify what needs to change in / be added to this PR? And let me know if you're happy with the GitHub URL I put for your name link?

@sappelhoff @cbrnr can you look to see if this makes sense in light of discussions in #8544?

Closes #8544
Closes #8551

@a-hurst
Copy link
Contributor

a-hurst commented Nov 23, 2020

Looks good, thanks for this! The part you're asking about refers to this section:

        # Populate edf_info
        edf_info.update(
            ch_names=ch_names, data_offset=header_nbytes,
            digital_max=digital_max, digital_min=digital_min,
            highpass=highpass, sel=sel, lowpass=lowpass, meas_date=meas_date,
            n_records=n_records, n_samps=n_samps, nchan=nchan,
            subject_info=patient, physical_max=physical_max,
            physical_min=physical_min, record_length=record_length,
            subtype=subtype, tal_idx=tal_idx)

In my local copy, I added recording_info=meas_id, after subject_info=patient, so that the recording info actually gets passed to the final raw object (before it was just discarded). I'm not sure if that's the proper naming convention or not, but I couldn't find any info on whether MNE standardized the naming and location of that kind of extended metadata.

Also, if you want to do some more extensive testing of the new header parsing you can base it off the tests I wrote for mne-bids here:

https://github.com/mne-tools/mne-bids/blob/18cc4f912495e8de8ab30d9c3077b19c923bebf4/mne_bids/tests/test_copyfiles.py#L116-L142

The MNE test .edf and .bdf files I looked at don't seem to have any of those header fields actually filled out, so manually adding the header info pre-test might be useful!

EDIT: To answer your other question, yes the Github link you used for me is fine!

Copy link
Contributor

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

LGTM, let me know if you want me to take a look at the missing feature once you've implemented it.

mne/io/edf/edf.py Outdated Show resolved Hide resolved
mne/io/edf/tests/test_edf.py Show resolved Hide resolved
mne/io/edf/tests/test_edf.py Outdated Show resolved Hide resolved
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

LGTM overall once Clemens' comments are taken into account

@larsoner
Copy link
Member Author

In my local copy, I added recording_info=meas_id, after subject_info=patient, so that the recording info actually gets passed to the final raw object (before it was just discarded).

It gets passed into a dict that ends up living only in a private attribute raw._raw_extras[0], so people should not rely on these values to exist (see extensive discussion here), so I'm -1 on adding these until we decide to make some EDF attributes public.

@larsoner larsoner changed the title WIP, BUG: Fix EDF header parsing MRG, BUG: Fix EDF header parsing Nov 23, 2020
@larsoner
Copy link
Member Author

@cbrnr made the tests more complete, should cover what you asked about

@cbrnr
Copy link
Contributor

cbrnr commented Nov 24, 2020

@larsoner did you push the changes?

@larsoner
Copy link
Member Author

nope, forgot the -f so it didn't go through -- it's there now, though!

@larsoner larsoner merged commit 2c5bf45 into mne-tools:master Nov 24, 2020
2 of 4 checks passed
@larsoner larsoner deleted the edf branch August 25, 2021 14:51
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.

BUG: Some tests require dipy >= 1.3 EDF/EDF+ headers parsed incompletely/incorrectly
4 participants