Fix chpi write #1067

eort · 2022-09-12T13:49:20Z

PR Description

closes #940
Given the added functionality in mne-python, I adapted the way the status of the hpi's is determined for neuromag files. Essentially, if any of the hpi's were active for at least one timepoint the field is set.

I wasn't sure what to do about the HeadCoilFrequency field. It seems like it is set regardless of the hpi status, but I think it is confusing (and rather pointless), to have the frequencies listed even though non of them were active. So I set them to None in case of inactive hpi.

Also the functionality would require the latest version of mne-python (1.2)

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

All comments are resolved
This is not your own PR
All CIs are happy
PR title starts with [MRG]
whats_new.rst is updated
New contributors have been added to CITATION.cff
PR description includes phrase "closes <#issue-number>"

eort · 2022-09-12T14:14:56Z

Also the functionality would require the latest version of mne-python (1.0.2)

which is at least partially responsible for the tests failing

agramfort · 2022-09-12T17:15:12Z

@eort can you make your PR draft if it’s not ready yet ? 🙏

sappelhoff · 2022-09-13T09:19:05Z

I wasn't sure what to do about the HeadCoilFrequency field. It seems like it is set regardless of the hpi status, but I think it is confusing (and rather pointless), to have the frequencies listed even though non of them were active. So I set them to None in case of inactive hpi.

agreed

Also the functionality would require the latest version of mne-python (1.2)

can you please add some logic such that this new feature is only used when an adequate version of mne-python is present, and else the old code is used? You can also add a comment like XXX: Remove this when support for mne <1.2 is dropped

Same for a test: just use the "skipif" decorator that you can already find in several other tests.

eort · 2022-09-13T11:55:05Z

can you please add some logic such that this new feature is only used when an adequate version of mne-python is present, and else the old code is used?

Yes, I can do that. I don't think leaving the old code makes a lot of sense though, because the old code gives incorrect information (always claiming chpi was present). But like Eric pointed out, the get_chpi_info method doesn't provide any info about whether the hpi were active or not. So for versions < 1.2 I would rather just not set those fields. This would then also have consequences for the tests, as neuromag files wouldn't be tested if the mne version is <1.2. I added similar version checks to the tests.

Same for a test: just use the "skipif" decorator that you can already find in several other tests.

I couldn't find any 'live' usage of this decorators. Regardless, can they be used if those version checks are only relevant for neuromag files? The behavior for CTF and KIT files hasn't change.

sappelhoff · 2022-09-13T12:13:01Z

The changes you pushed LGTM, thanks! If you could also add a changelog entry, I think this becomes mergeable. We just need to inspect the CI log very well to make sure no "new issues" are introduced, and errors are all due to expected shortcomings (see discussion in #1066)

eort · 2022-09-13T12:55:16Z

Talking about introducing new issues... What is preferred behavior in case it cannot be determined whether a field is True or False? Is it setting it to None or leaving it out altogether? So far, it was left out, but my understanding was setting it to None

sappelhoff · 2022-09-13T12:59:51Z

in the case of missing data, if it is metadata that BIDS considers optional, leaving it out completely is preferred IMHO.

If it is recommended and we don't have info, we need to set it to n/a, I think.

eort · 2022-09-13T13:15:01Z

Both HeadCoilFrequency and ContinuousHeadLocalization are recommended, so I set them to n/a in case of missing info

eort · 2022-09-13T15:06:17Z

Turns out things are more complicated than I thought.

'n/a' is not a valid value for HeadCoilFrequency and ContinuousHeadLocalization (which is probably the reason they were left out in first place). So, we either change the bids specifications such that "n/a" will be allowed, or we only use those allowed values (bool for ContinuousHeadLocalization, and numbers/lists of numbers for HeadCoilFrequency. I lean towards the first option. Opinions?
the new method get_active_chpi calls get_chpi_info
in case of missing hpi data this will throw an error. While latter method has on-missing argument, so we could add that argument to get_active_chpi and pass it through to get_chpi_info. Or are there better options?

sappelhoff · 2022-09-13T15:24:32Z

Ah, I see. I think it should be possible to specify n/a so I also lean towards your first option. Else we can continue with omitting these metadata.

eort · 2022-09-14T13:33:37Z

mh, after checking again the specifications, I came across this passage:

It is RECOMMENDED that non-compulsory metadata fields (like notch in channels.tsv files) and/or files (like events.tsv) are fully omitted when they are unavailable or unapplicable, instead of specified with an n/a value, or included as an empty file (for example an empty events.tsv file with only the headers included).

Given that we are talking about optional fields here, I think adding n/a is not ideal, instead the field should just not be set when no information on the status are available.

sappelhoff · 2022-09-14T13:35:45Z

😮‍💨 shame on me, because I was the one adding that sentence to the spec. And I didn't remember when I wrote what I did above. Thanks for double checking @eort

eort · 2022-09-14T13:48:04Z

no prob. Seems unrealistic to remember all details of a 500 pages doc ;)

So only thing that needs to be done still, is the api change in mne.chpi.get_active_hpi(). Once that is merged, I will push the latest version to this PR.

doc/whats_new.rst

mne_bids/tests/test_write.py

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

…o fix_chpi_write

eort · 2022-09-16T10:43:21Z

there are still problems in mne.chpi.get_active_chpi(). Once they are solved, I shall try once again to get this thing to work

codecov · 2022-09-19T05:42:51Z

Codecov Report

Merging #1067 (52acc36) into main (07af8cc) will decrease coverage by 0.18%.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##             main    #1067      +/-   ##
==========================================
- Coverage   95.20%   95.01%   -0.19%     
==========================================
  Files          25       24       -1     
  Lines        3840     3835       -5     
==========================================
- Hits         3656     3644      -12     
- Misses        184      191       +7

Impacted Files	Coverage Δ
mne_bids/write.py	`96.79% <70.58%> (-0.39%)`	⬇️
mne_bids/read.py	`95.36% <0.00%> (-0.74%)`	⬇️
mne_bids/__init__.py

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

eort · 2022-09-19T08:36:09Z

I marked this ready for review, because I don't think the CI failures are related to the introduced changes. Also code coverage decrease is not a real decrement in my opinion, at least I don't see obvious ways to increase it.

sappelhoff

LGTM, thanks @eort for working on this and the added functionality in mne-python!

sappelhoff · 2022-09-27T11:49:55Z

oh but actually, we do get one error that seems related, see: https://github.com/mne-tools/mne-bids/actions/runs/3080131213/jobs/4977127367#step:14:5540

E NotImplementedError: Identifying active HPI channels is not implemented for other systems than neuromag.

found an issue with tests

eort · 2022-09-27T12:44:51Z

oh indeed. I missed that among all the matplotlib errors.

I am confused about the message though. It says that it fails to properly handle a RawCNT file, but in the configuration of the test, only ('fif_no_chpi', 'fif', 'ctf', 'kit') are listed. Where does the file come from?

In any case, the problem seems to be that all system that are not KIT or CTF are lumped into one category, assuming those must be neuromag then. So, we could change it to an elif, and throw and notimplementederror for the else case?

sappelhoff · 2022-09-27T13:10:55Z

In any case, the problem seems to be that all system that are not KIT or CTF are lumped into one category, assuming those must be neuromag then. So, we could change it to an elif, and throw and notimplementederror for the else case?

yes, seems like it -- I don't know how (would need to read more carefully) but it seems like you need to fix exactly that logic.

eort · 2022-09-27T14:03:10Z

I used the same logic as in https://github.com/mne-tools/mne-python/blob/de546e2188761de69eede103b7c5a6cc09542a4d/mne/chpi.py#L1392 and restructured the entire if-else section accordingly.

eort · 2022-09-27T14:34:02Z

I don't see anything wrong with the reported problems, but better check for yourself

sappelhoff · 2022-09-28T17:09:54Z

Thank you @eort 🎉

eort added 2 commits September 12, 2022 15:33

fix determination of cHPI status

f50f998

replace None with empty list

ce18571

sappelhoff added the enhancement label Sep 12, 2022

sappelhoff added this to the 0.11 milestone Sep 12, 2022

eort marked this pull request as draft September 12, 2022 21:36

add version checks

21d473b

fix style

1ef90f7

handle missing data

c65d604

handle missing data

1b8bcd9

add proper test

c2dc520

eort mentioned this pull request Sep 14, 2022

getting chpi information crashes when no hpi information are available mne-tools/mne-python#11162

Closed

add changelog entry

97c3d20

hoechenberger reviewed Sep 15, 2022

View reviewed changes

doc/whats_new.rst Outdated Show resolved Hide resolved

mne_bids/tests/test_write.py Outdated Show resolved Hide resolved

mne_bids/tests/test_write.py Outdated Show resolved Hide resolved

eort and others added 4 commits September 16, 2022 09:22

Apply suggestions from code review

8aa41ad

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne_bids/tests/test_write.py

7755992

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

Update mne_bids/tests/test_write.py

b9a9a60

Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>

make version distinction in test_read

de912a0

eort added 2 commits September 16, 2022 09:59

Merge branch 'fix_chpi_write' of https://github.com/eort/mne-bids int…

0320894

…o fix_chpi_write

add import parseVersion

217ec51

handle no chpi files

5467ba8

eort marked this pull request as ready for review September 19, 2022 08:34

sappelhoff previously approved these changes Sep 27, 2022

View reviewed changes

be more specific when choosing the meg system

52acc36

sappelhoff merged commit 22cbd10 into mne-tools:main Sep 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix chpi write #1067

Fix chpi write #1067

eort commented Sep 12, 2022 •

edited by sappelhoff

Loading

eort commented Sep 12, 2022

agramfort commented Sep 12, 2022

sappelhoff commented Sep 13, 2022

eort commented Sep 13, 2022

sappelhoff commented Sep 13, 2022

eort commented Sep 13, 2022

sappelhoff commented Sep 13, 2022

eort commented Sep 13, 2022 •

edited

Loading

eort commented Sep 13, 2022 •

edited by sappelhoff

Loading

sappelhoff commented Sep 13, 2022

eort commented Sep 14, 2022

sappelhoff commented Sep 14, 2022

eort commented Sep 14, 2022

eort commented Sep 16, 2022

codecov bot commented Sep 19, 2022 •

edited

Loading

eort commented Sep 19, 2022

sappelhoff left a comment

sappelhoff commented Sep 27, 2022

eort commented Sep 27, 2022

sappelhoff commented Sep 27, 2022

eort commented Sep 27, 2022

eort commented Sep 27, 2022 •

edited

Loading

sappelhoff commented Sep 28, 2022

Fix chpi write #1067

Fix chpi write #1067

Conversation

eort commented Sep 12, 2022 • edited by sappelhoff Loading

PR Description

Merge checklist

eort commented Sep 12, 2022

agramfort commented Sep 12, 2022

sappelhoff commented Sep 13, 2022

eort commented Sep 13, 2022

sappelhoff commented Sep 13, 2022

eort commented Sep 13, 2022

sappelhoff commented Sep 13, 2022

eort commented Sep 13, 2022 • edited Loading

eort commented Sep 13, 2022 • edited by sappelhoff Loading

sappelhoff commented Sep 13, 2022

eort commented Sep 14, 2022

sappelhoff commented Sep 14, 2022

eort commented Sep 14, 2022

eort commented Sep 16, 2022

codecov bot commented Sep 19, 2022 • edited Loading

Codecov Report

eort commented Sep 19, 2022

sappelhoff left a comment

Choose a reason for hiding this comment

sappelhoff commented Sep 27, 2022

eort commented Sep 27, 2022

sappelhoff commented Sep 27, 2022

eort commented Sep 27, 2022

eort commented Sep 27, 2022 • edited Loading

sappelhoff commented Sep 28, 2022

eort commented Sep 12, 2022 •

edited by sappelhoff

Loading

eort commented Sep 13, 2022 •

edited

Loading

eort commented Sep 13, 2022 •

edited by sappelhoff

Loading

codecov bot commented Sep 19, 2022 •

edited

Loading

eort commented Sep 27, 2022 •

edited

Loading