FIX: Handling bad marker coils when reading KIT data #8981 by JD-Zhu · Pull Request #8989 · mne-tools/mne-python

JD-Zhu · 2021-03-04T01:06:14Z

When reading .con files from KIT system, an error occurs if there are missing marker coils. The proposed solution here resolves this problem by checking whether a marker coil exists before retrieving its coordinates.

…hether a marker coil exists before retrieving its coordinates.

JD-Zhu · 2021-03-04T04:37:02Z

@agramfort I think this is ready for review.

It's my first contribution - sorry about the commit messages, I didn't realise the tags should be included in each commit, rather than just the PR title.

agramfort · 2021-03-04T08:35:49Z

mne/io/kit/kit.py

+                if key in dig:
+                    elp.append(dig.pop(key))
            if 'hpi_5' in dig and dig['hpi_5'].any():
                elp.append(dig.pop('hpi_5'))


it seems this if just below is exactly to catch the issue of missing hpi_5. Maybe we can use the same logic for all hpi's ?

Yes I think that's what it's doing, but not sure why it was checking specifically for hpi_5 and not the others.

Also, based on the additional condition dig['hpi_5'].any(), it seems like [0, 0, 0] is considered as invalid coordinates. Again not sure if this is something specific to hpi_5 or should be checked for all marker coils. I don't think we should treat [0, 0, 0] as invalid in general though.

Yes I think that's what it's doing, but not sure why it was checking specifically for hpi_5 and not the others.

I added this because there was one dataset I think was from @paulsowman that had the first four only. But I suppose it makes sense that you might be missing any one (or up to two?) of the five.

It looks like this special case is no longer needed. All hpi_* should be treated the same way.

While unifying it, we can try getting rid of the .any and see if it breaks anything. Maybe np.isfinite(dig['hpi_5']).all() is a better check.

Agreed and implemented. Doesn't seem to break anything.

drammock · 2021-03-04T13:59:08Z

It's my first contribution - sorry about the commit messages, I didn't realise the tags should be included in each commit, rather than just the PR title.

Hooray for your first contribution!! FYI, it's not crucial to put tags like sty or doc in every commit message if the messages are descriptive (like yours are). When we merge PRs we squash all commits into one anyway. Your commit messages are better than many of mine, which often just say "missed one" or "try again" 😆

agramfort · 2021-03-04T14:03:43Z

I don't have enough test KIT files so maybe we should just take your fix here and it seems the less likely to introduce a regression

…

JD-Zhu · 2021-03-04T22:53:32Z

Thanks @larsoner Indeed we have another dataset with two missing coils. The present fix cannot handle that situation yet, as it would fail at the assertion a few lines below: assert elp.shape in ((7, 3), (8, 3)) In the case of two missing coils, the shape becomes (6, 3). I tried removing this assertion temporarily, but it seems to run into more problems later on.

________________________________ From: Eric Larson <notifications@github.com> Sent: Friday, March 5, 2021 3:13:22 AM To: mne-tools/mne-python <mne-python@noreply.github.com> Cc: Di Zhu <di.zhu@mq.edu.au>; Author <author@noreply.github.com> Subject: Re: [mne-tools/mne-python] FIX: Handling bad marker coils when reading KIT data #8981 (#8989) @larsoner commented on this pull request.

________________________________ In mne/io/kit/kit.py<#8989 (comment)>:

@@ -720,9 +720,12 @@ def get_kit_info(rawfile, allow_unknown_format, standardize_names=None,

hsp.append(rr) # nasion, lpa, rpa, HPI in native space - elp = [dig.pop(key) for key in ( - 'fidnz', 'fidt9', 'fidt10', - 'hpi_1', 'hpi_2', 'hpi_3', 'hpi_4')] + elp = [] + for key in ( + 'fidnz', 'fidt9', 'fidt10', + 'hpi_1', 'hpi_2', 'hpi_3', 'hpi_4'): + if key in dig: + elp.append(dig.pop(key)) if 'hpi_5' in dig and dig['hpi_5'].any(): elp.append(dig.pop('hpi_5')) Yes I think that's what it's doing, but not sure why it was checking specifically for hpi_5 and not the others. I added this because there was one dataset I think was from @paulsowman<https://github.com/paulsowman> that had the first four only. But I suppose it makes sense that you might be missing any one (or up to two?) of the five. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#8989 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AJE5HU5VJVDP2Q74VT6GP5LTB6WSFANCNFSM4YSI5ECA>.

larsoner · 2021-03-04T23:10:04Z

That's an assert statement I put in to make sure that, if someone changed the behavior, they look at the downstream impact. Let me know if you need help assessing it. But I think checking that at least 3 are available, and maybe indexing of the relevant ones (I might have coded it to assume the last coil was the missing one?) will be necessary

JD-Zhu · 2021-03-05T04:05:19Z

That's an assert statement I put in to make sure that, if someone changed the behavior, they look at the downstream impact. Let me know if you need help assessing it. But I think checking that at least 3 are available, and maybe indexing of the relevant ones (I might have coded it to assume the last coil was the missing one?) will be necessary

Thanks for your help @larsoner
I changed the assert to allow a minimum of 3 marker coils (+ 3 fiducials), and did the same for a similar check in mne\io\kit\coreg.py (line 151). This seems to work fine without further issues.
For the indexing of relevant marker coils, I think this is already provided by their names (hpi_1, hpi_2, etc)?

JD-Zhu · 2021-03-05T05:19:35Z

One of the tests failed - I can't figure out what it means / how to fix it. Would appreciate any advice!

larsoner · 2021-03-05T13:05:17Z

mne/io/kit/kit.py

            elp = np.array(elp)
            hsp = np.array(hsp, float).reshape(-1, 3)
-            assert elp.shape in ((7, 3), (8, 3))
+            assert elp.shape in ((6, 3), (7, 3), (8, 3))


Now that it's possible for a user (as opposed to only a dev making code chainges) to run into a situation where this could be violated, this should become a proper check and raise, e.g.:

Suggested change

assert elp.shape in ((6, 3), (7, 3), (8, 3))

if elp.shape not in ((6, 3), (7, 3), (8, 3)):

raise RuntimeError(f

f'Fewer than 3 HPI coils found, got {len(elp) - 3}')

Implemented the suggested change.

…out of 5) coils.

agramfort

diff LGTM

can you merge again the main branch to see if it fixes the CIs. The +1 for merge when green

doc/changes/names.inc

JD-Zhu · 2021-03-08T02:27:16Z

Still 1 failed test. Any idea what the issue might be?

agramfort · 2021-03-08T07:35:15Z

it does not seem related. Let's wait for us to fix this other issue and then we'll merge this one. thx

…

JD-Zhu · 2021-03-08T08:17:52Z

Sure 😃

doc/changes/names.inc

larsoner · 2021-03-09T13:46:15Z

Thanks @JD-Zhu !

JD-Zhu · 2021-03-10T23:14:28Z

Thanks @larsoner @agramfort @drammock
Happy to contribute!

JD-Zhu added 5 commits March 4, 2021 11:14

Resolves the error when there are missing marker coils, by checking w…

44f76d4

…hether a marker coil exists before retrieving its coordinates.

Resolves the error when there are missing marker coils, by checking w…

2e27134

…hether a marker coil exists before retrieving its coordinates.

Fixed code style issues.

e992736

Fixed code style issues.

bc29465

Fixed build_docs error.

1f16822

agramfort reviewed Mar 4, 2021

View reviewed changes

Allow up to 2 missing marker coils

b99a291

JD-Zhu added 3 commits March 5, 2021 15:08

Resolved conflict in names.inc

b325111

Resolve merge conflict

207bbae

Resolve merge conflict

9b33ea9

larsoner reviewed Mar 5, 2021

View reviewed changes

JD-Zhu added 2 commits March 6, 2021 10:30

Merge remote-tracking branch 'upstream/main' into master

f1c2b2c

Unified the checking of all marker coils. Raise error if few than 3 (…

b37b3fc

…out of 5) coils.

agramfort reviewed Mar 7, 2021

View reviewed changes

doc/changes/names.inc Outdated Show resolved Hide resolved

JD-Zhu added 2 commits March 8, 2021 12:29

Merge remote-tracking branch 'upstream/main' into master

e9e43cc

Added empty line at end of file

908e0d0

larsoner reviewed Mar 8, 2021

View reviewed changes

doc/changes/names.inc Show resolved Hide resolved

JD-Zhu added 2 commits March 9, 2021 09:57

Merge remote-tracking branch 'upstream/main' into master

28b0813

Added a line to changelog

469c56a

larsoner merged commit 31d1d0c into mne-tools:main Mar 9, 2021

JD-Zhu mentioned this pull request Jan 30, 2024

ENH: Allow removal of (up to two) bad marker coils in read_raw_kit() #12394

Merged

-            assert elp.shape in ((6, 3), (7, 3), (8, 3))
+            if elp.shape not in ((6, 3), (7, 3), (8, 3)):
+                raise RuntimeError(f
+                    f'Fewer than 3 HPI coils found, got {len(elp) - 3}')

Uh oh!

Conversation

JD-Zhu commented Mar 4, 2021

Uh oh!

JD-Zhu commented Mar 4, 2021

Uh oh!

agramfort Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

JD-Zhu Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

larsoner Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

larsoner Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

JD-Zhu Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

drammock commented Mar 4, 2021

Uh oh!

agramfort commented Mar 4, 2021 via email

Uh oh!

JD-Zhu commented Mar 4, 2021 via email

Uh oh!

larsoner commented Mar 4, 2021

Uh oh!

JD-Zhu commented Mar 5, 2021

Uh oh!

JD-Zhu commented Mar 5, 2021

Uh oh!

larsoner Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

JD-Zhu Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JD-Zhu commented Mar 8, 2021

Uh oh!

agramfort commented Mar 8, 2021 via email

Uh oh!

JD-Zhu commented Mar 8, 2021

Uh oh!

Uh oh!

larsoner commented Mar 9, 2021

Uh oh!

JD-Zhu commented Mar 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants