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

Digitization progress #6461

Closed
6 of 19 tasks
massich opened this issue Jun 17, 2019 · 19 comments
Closed
6 of 19 tasks

Digitization progress #6461

massich opened this issue Jun 17, 2019 · 19 comments
Milestone

Comments

@massich
Copy link
Contributor

massich commented Jun 17, 2019

This issue is to keep track of the remaining Digitization PRs. It reflects a conversation with @larsoner.

TODO (each item will be a separate PR):

  • Add move all montage use to Raw and Epochs readers that have montage argument to call only the following at the end of their __init__, not used anywhere else (Add montage parameter to BaseRaw #6462):

    if montage is not None:
        self.set_montage(montage)
    
  • Deprecate montage argument in all readers that support montage arguments ([MRG] Deprecate montage param from all EEG raw readers #6534)

    • Make we have a test like sure Digitization progress #6461 (comment)
       def test_montage():
       	raw_montage = read_raw_*(fname, montage=my_montage)
           raw_none = read_raw_*(fname, montage=None)
           raw_none.set_montage(montage)
           assert object_diff(raw_none.info['chs'], raw_montage.info['chs'])
       	assert object_diff(raw_none.info['dig'], raw_montage.info['dig'])
  • Replicate montage and digmontage usecases with Digitization This no longer applies. We kept DigMontage, remove Montage and make digitization private.

    • Refactor DigMontage to use Digitization + ch_names as representation (+ deprecate things that we no longer want to support) (MNT: Change DigMontage representation to use Digitization #6639)
    • Make digitization module private _digitization (Make mne/digitization private mne/_digitization #6700)
    • Fix DigMontage.save (saves dig but not ch_names)
    • Fix DigMontage() without parmeters.
    • Refactor DigMontage tests. All that we did not change in MNT: Change DigMontage representation to use Digitization #6639 because we only wanted to touch code logic not test logic.
    • set_montage takes only DigMontage not Montage
    • Some refactor of mne/_digitization. We have all _make_dig_{kit, artemist, bti} that overlap with DigMontage.
    • deprecate read_dig_montage in favor of read_dig_montage_FOO or make_dig_montage(np.arrays..). No transforms allowed.
    • deprecate DigMontage.dev_head_t in favor of:
      • Storing the 2 versionsof the HPI points in the DigMontage.dig: one in FIFFV_COORD_HEAD and the other in FIFFV_COORD_MEG.
        When you want dev_head_t, call DigMontage.get_dev_head_t() and it will compute it from these directly.
      • a free function returning a proper transformation: def compute_dev_head_t(dig: Digitization) -> Transform:
    • rename elp, hpi in favor of the new names (hpi, hpi_dev) in the internal calls. (I'm not sure this will be needed. We'll see)
    • DigMontage.transform_to_head() (not fully clear but it will get more clear when addressing the others). Things that could require:
      • a free function like: def get_fiducials(dig: Digitization) -> Fiducials: (Fiducials is a Bunch with nasion, lpa, rpa or similar, named tuple whatever.)
      • do all points need to be in 'head' or only some of them?
      • does it need to be a method? (probably yes)
  • remove fit_match_points outside of coreg.

  • move read_dig_XXXX to mne/io/

  • Add plot() to Digitization class WIP: Add plot method to digitization #6412. 3D plot with each dig point, maybe symbol based on coordinate frame (sphere=head, cube=meg, ... ?) and color based on type (eeg, extra, hpi, we have colors for these in defaults.py or plot_alignment somewhere) Fix the DigMontage-Montage mess we had in the existing plot with point_names and custom transforms. ([RFC] Are we aiming to be able to write plot_montage like this? #6649, maybe different PR)

  • Make a none smoke test for applied transforms Reveal missing test massich/mne-python#31 (I've close it by error, it still applies)


Log of past PRs:

Cross-reference issues:


Here is a summary of the readers with respect to how they are tested. Main focus: does the reader go through _test_raw_reader? has the reader been tested with digitization or None? Does the reader have digitization or a montage?

                       | _test_raw_reader | None | Digitization | other   |
-----------------------+------------------+------+--------------+---------+
RawArray               |     x            |  x   |              |         |
read_raw_artemis123    |     x            |      |     x        |         |   
read_raw_bdf           |                  |      |              | montage |
read_raw_brainvision   |     x            |  x   |     x        |         |    
read_raw_bti           |     x            |  x   |     x        |         |
read_raw_cnt           |     x            |  x   |              | montage |
read_raw_ctf           |     x            |      |     x        |         |
read_raw_edf           |     x            |  x   |     x        |         |
read_raw_eeglab        |     x            |      |     x        |         |
read_raw_egi           |     x            |  x   |              | montage |
read_raw_eximia        |     x            |  x   |              | ------- |
read_raw_fieldtrip     |                  |      |              | ------- |
read_raw_fif           |     x            |      |     x        |         |
read_raw_gdf           |     x            |  x   |              | montage |
read_raw_kit           |     x            |  x   |     x        |         |
read_raw_nicolet       |     x            |  x   |              | montage |

other notes:

  1. duplicated montages
import os.path as op
import mne
fname = op.join(op.dirname(mne.io.edf.__file__), 'tests', 'data', 'biosemi.hpts')

default_biosemi64 = mne.channels.read_montage('biosemi64')
edf_testing_biosemi64 = mne.channels.read_montage(fname)
@GuillaumeFavelier
Copy link
Contributor

Thanks for the overview @massich! I think there is also #6412 (add a plot() function to the Digitization class). I still have to integrate @agramfort reviews.

@massich
Copy link
Contributor Author

massich commented Jun 17, 2019

@GuillaumeFavelier right!

@larsoner
Copy link
Member

@massich updated top description with my suggested order / description based on extrapolating what I discussed with @agramfort

@massich
Copy link
Contributor Author

massich commented Jun 19, 2019

just to make sure that we are all on the same page. This should pass without warning error nothing:

def test_montage():
    raw_montage = read_raw_*(fname, montage=my_montage)
    raw_none = read_raw_*(fname, montage=None)
    raw_none.set_montage(montage)
    assert object_diff(raw_none.info['chs'], raw_montage.info['chs'])
    assert object_diff(raw_none.info['dig'], raw_montage.info['dig'])

@agramfort
Copy link
Member

agramfort commented Jun 19, 2019 via email

@larsoner
Copy link
Member

Yes in this PR. Eventually in this release cycle we will deprecade the read_raw_*(fname, montage=my_montage), but not in this PR. (It should be almost trivial after this PR.)

@larsoner
Copy link
Member

Sorry, I meant to say that yes, that should be the behavior in #6462.

In the PR after that, read_raw_*(fname, montage=my_montage) should be deprecated and the user told to do raw.set_montage(my_montage).

@massich
Copy link
Contributor Author

massich commented Jun 26, 2019

update_ch_names has been exposed in 0.19. Once Digitization is here, we should be able to remove it from the signature.

def set_montage(self, montage, set_dig=True, update_ch_names=False,

In other words, Digitization should go in before releasing so that we don't need to deprecate this parameter.

@massich
Copy link
Contributor Author

massich commented Jul 10, 2019

now that montage is deprecated, the idea is that raw.set_montage only takes Digitization and that as a middle step montages and digmontages had to be transformed to Digitizations, am I right? @agramfort, @larsoner

@larsoner
Copy link
Member

Yeah I think so.

@agramfort
Copy link
Member

agramfort commented Jul 10, 2019 via email

@massich
Copy link
Contributor Author

massich commented Jul 11, 2019

set_montage sounds fine to me

@massich
Copy link
Contributor Author

massich commented Jul 11, 2019

There are at least two ways of getting the digitization information from a fif file:

dig = _read_dig_fif(fid, meas_info)

if fif is not None:

it is true that one populates meas_info while the other returns a DigMontage, but they are essentially the same.

(The later calls the former)

@agramfort
Copy link
Member

agramfort commented Jul 11, 2019 via email

@massich
Copy link
Contributor Author

massich commented Aug 26, 2019

Regarding the kind we have this FIF.FIFV_MNE_COORD_DIGITIZER and FIFFV_COORD_ISOTRAK that is used nowhere.

~/code/mne-python read_dig_montage_polhemus
(mne) ❯ git grep -i 'FIFFV_MNE_COORD_DIGITIZER'
mne/io/constants.py:FIFF.FIFFV_MNE_COORD_DIGITIZER   = FIFF.FIFFV_COORD_ISOTRAK # Original (Polhemus) digitizer coordinates
mne/io/tests/test_constants.py:    FIFFV_MNE_COORD_DIGITIZER='FIFFV_COORD_ISOTRAK',

@agramfort
Copy link
Member

agramfort commented Aug 26, 2019 via email

@teonbrooks
Copy link
Member

explaining some context behind the KIT digitization process at NYU based on a discussion with @massich.

so for the KIT system, the use of the labels elp and hpi had come from the naming used in the old mne-c manual.

HPI
Head Position Indicator (HPI) points come from the head localization procedure in the MEG with the attached coils. There are five coils for the KIT system, these points have a specific order, and that this allows for the coregistration of the head into MEG device space for the dev_head_t computation. This is stored in either a .mrk file or a .sqd file, which just contains this head localization procedure.

ELP
The electrode points file has been constructed to emulate the .elp file structure that was produced from the old ISOTRAK pen IIRC. In this file, there is a section for the fiducials which exists at the top, and there is a section for the corresponding HPI points but collected in the digitizer native space.
The NYU lab with the KIT system has been accustomed to include all 8 points in an exported txt file from the FASKTRAK digitizer. This has been done as a matter of lab protocol convention.

I think that ideally there should be three files as opposed to two files: one for the HPI points (5 points), one for those points in digitizer space (5 points), and one file for the fiducials (3 points). I don't think including the fiducials in the points file is a good idea anymore because this requires an arbitrary assignment of points to references. From my discussion with @massich, it is true that someone could place the points in any manner they sought fit but there is no way for the software to intelligently know this.

File Format
FASKTRAK allows for exports to txt files but there isn't anything inherently special about it, it just arbitrary number of point x 3. These points are referenced to a reference box therefore all the digitizer data must be re-reference to neuromag head space based on fiducials. The polhemus file reader is just one that ignores the header and just reads the data in as an array.

Naming
As for the names of HPI and ELP, there has been a proposal to rename the points:
HPI --> hpi_dev
ELP --> hpi

After thinking about this, I think that HPI should remain being hpi and ELP should be the one renamed to something like hpi_isotrak or hpi_isotrak_space
HPI --> hpi
ELP --> hpi_isotrak

hope this provides some context and recommendations.

@larsoner
Copy link
Member

@massich do we need any of the rest of these for 0.19? Or just #6764?

@massich
Copy link
Contributor Author

massich commented Sep 23, 2019

I think #6764 is enough. I'm closing this one

@massich massich closed this as completed Sep 23, 2019
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

No branches or pull requests

5 participants