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, MAINT: Remove _read_dig_points #8622

Merged
merged 2 commits into from Dec 8, 2020
Merged

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Dec 7, 2020

  • Remove mne.io._digitization._read_dig_points and _set_dig_kit in favor of mne.io.kit.coreg._read_dig_kit and _set_dig_kit. The first function just reads files based on names that typically (I'm assuming) come with KIT machines to work the same way as it did before with KIT coreg, etc. The second does transforms necessary to get in the correct coondinate system. This is similar to how things work with CTF, and makes _digitization cleaner. The _kit functions now only show up in io/kit and gui/*kit*.
  • Add on_header_missing to read_polhemus_fastscan because some of our testing files and such do not produce this header, and it's possible there are files in the wild that the KIT people are using that have this feature.

Part of #7591. @christianbrodbeck can you look since this messes with KIT?

@agramfort
Copy link
Member

this goes a bit backwards with our earlier attempt with @massich to centralize the logic on dig

why moving things to kit rather and consolidation dig specific file?

@larsoner
Copy link
Member Author

larsoner commented Dec 7, 2020

why moving things to kit rather and consolidation dig specific file?

The idea is that:

  1. standard handling of digitization data or things that are useful to many readers (file formats, converting from unknown to head, etc.) should live in mne/channels, and that
  2. raw-reader-specific mangling can be handled in their own modules.

It's a better split between what is standard dig information and what is specific to a format. For example, before this PR I had no idea why _read_dig_points even needed to exist. In this PR the functionality is clearly separated (and code commented) to make clear that the elp/mrk etc that get passed to read_raw_kit assume for example that

  1. Any .txt should be read like a Polhemus FastScan
  2. Any .elp and .hsp should be read like a Polhemus Isotrak (and not BESA elp)
  3. The outputs should be ordered nasion/lpa/rpa instead of lpa/nasion/rpa (Neuromag order) to work with KIT coreg functions.

To me this is analogous to how we handle CTF data and coordinate frames: https://github.com/mne-tools/mne-python/blob/master/mne/io/ctf/trans.py#L43-L114

@larsoner larsoner merged commit 90f4dec into mne-tools:master Dec 8, 2020
2 checks passed
@larsoner larsoner deleted the dig branch December 8, 2020 12:41
@massich
Copy link
Contributor

massich commented Dec 8, 2020

Our initial idea was to move it to a centralized place, and we used kit to get hold of the whole thing. Once kit was there we could factor out lots of things that were common, and some of them we could not. If I recall properly, this was what got left from the original kit. It never made it back to kit 'cos I always believed that I could manage to factor the whole thing out and bring back something smaller. And apply a fancy programming pattern to rule them all.

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.

None yet

3 participants