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

MNT: Change DigMontage representation to use Digitization #6639

Merged
merged 41 commits into from
Aug 22, 2019

Conversation

massich
Copy link
Contributor

@massich massich commented Aug 7, 2019

The goal of this PR is:
1 - to change the internal represenation of mne.channels.DigMontage to use Digitization and ch_names only.
2 - remove mne.channels.DigMontage._get_dig method.
3 - update set_montage to use the new internal representation.

Things to consider:

  • No public API change
  • In an ideal world, no test should change. If that's not the case, that would show us what is missing/wrong.

cc: @agramfort

cross-ref: #6461

requires:


possible follow up PRs:

  • 1 pass in the tests
  • different readers
  • set_montage take dig_montage only
  • unify _make_dig_bla in mne/digitization
  • dev_head_t make it instance of Transform rather than np.array
  • move mne/digitization to mne/_digitization
  • remove fit_match_points outside of coreg.
  • rename elp, hpi in favor of the new names (hpi, hpi_dev) in the internal calls.

@massich massich force-pushed the change_dig_montage_representation branch from 66b166a to 8c3fe60 Compare August 8, 2019 07:42
@codecov
Copy link

codecov bot commented Aug 8, 2019

Codecov Report

Merging #6639 into master will increase coverage by <.01%.
The diff coverage is 88.85%.

@@            Coverage Diff             @@
##           master    #6639      +/-   ##
==========================================
+ Coverage   89.42%   89.42%   +<.01%     
==========================================
  Files         416      417       +1     
  Lines       75266    75405     +139     
  Branches    12356    12369      +13     
==========================================
+ Hits        67304    67429     +125     
- Misses       5147     5157      +10     
- Partials     2815     2819       +4

mne/channels/montage.py Outdated Show resolved Hide resolved
@agramfort
Copy link
Member

what's next? make sure that only self.dig and channel names are used in the set_montage method.

@massich
Copy link
Contributor Author

massich commented Aug 9, 2019

@agramfort sure, first we need to make sure that removing the attributes themselves is safe and we break nothing (this is massich#30), then update set_montage make sure that plot does not break. It should not.

Once all this is done we can do two things: either merge this and do a following PR were we merge all the functions I've strip out from the code with already existing code. (Just to keep the diff bounded) or we can do it here.

@massich
Copy link
Contributor Author

massich commented Aug 9, 2019

But now things are linear again, so once massich#30 is green I can merge it and keep doing commits here rather than PRs in my fork.

mne/channels/montage.py Outdated Show resolved Hide resolved
@agramfort
Copy link
Member

next we'll need to a read_montage_fif, read_montage_polhemus and a read_montage_egi. But I guess in a follow up PR.

@massich massich force-pushed the change_dig_montage_representation branch from 5cb430e to 45f5a6e Compare August 19, 2019 09:34
@massich
Copy link
Contributor Author

massich commented Aug 19, 2019

head transforms are not properly tested. see massich#31

mne/channels/montage.py Outdated Show resolved Hide resolved
s = ('<DigMontage | %d extras (headshape), %d HPIs, %d fiducials, %d '
'channels>' %
(len(self.hsp) if self.hsp is not None else 0,
(len(_data.hsp) if _data.hsp is not None else 0,
len(self.point_names) if self.point_names is not None else 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we use point_names as HPI?

cc: @agramfort

fname,
_raise_transform_err,
_all_data_kwargs_are_none,
):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

similar to

def _read_dig_fif(fid, meas_info):

Copy link
Member

Choose a reason for hiding this comment

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

Why change how we write functions here? Everywhere else in MNE we do:

def func(a, b, c):

We also never use underscore vars for args/kwargs. For codebase consistency please fix these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. But this is only here for a really short period of time. This function were just for convenience, and to keep track of all internal things for the refactoring.

I can change them to not _foo. But they day in the next PR.

self.dig = _make_dig_points(
nasion=nasion, lpa=lpa, rpa=rpa, hpi=elp,
extra_points=hsp, dig_ch_pos=dig_ch_pos
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

# XXX: all points are supposed to be in FIFFV_COORD_HEAD

@agramfort
Copy link
Member

@larsoner we are happy to announce that you can now have fun reviewing this one !!! 🍻

from .. import Info
# Update the backend
from .backends.renderer import _Renderer

if fig is None:
raise ValueError('The figure must have a scene')
if isinstance(montage, (Montage, DigMontage)):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was only referring to DigMontage since Montage has no dig_ch_pos. (I guess that this is how the code was intended. see the error raising has no reference to Montage)

@massich
Copy link
Contributor Author

massich commented Aug 21, 2019

does anyone know which docstring is CircleCI pointing at?

/home/circleci/project/mne/channels/montage.py:docstring of mne.channels.DigMontage:57: WARNING: py:obj reference target not found: Digitization
/home/circleci/project/mne/channels/montage.py:docstring of mne.channels.DigMontage:67: WARNING: py:obj reference target not found: strings

@larsoner
Copy link
Member

Okay code is too complex for a quick look. Will look deeper probably tomorrow morning

massich added a commit to massich/mne-python that referenced this pull request Aug 22, 2019
@massich
Copy link
Contributor Author

massich commented Aug 22, 2019

@larsoner adding make_dig_montage to whatsnew is missing but I'll do it after your review

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Can someone succinctly describe why sticking with DigMontage but deprecating all params to it is preferred over using a new class altogether? It seems like whatever we are using DigMontage for after this PR is what I had in mind that Digitization could do. And I don't see the advantages/disadvantages of the two approaches clearly

fname,
_raise_transform_err,
_all_data_kwargs_are_none,
):
Copy link
Member

Choose a reason for hiding this comment

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

Why change how we write functions here? Everywhere else in MNE we do:

def func(a, b, c):

We also never use underscore vars for args/kwargs. For codebase consistency please fix these

mne/channels/_dig_montage_utils.py Show resolved Hide resolved
mne/channels/_dig_montage_utils.py Show resolved Hide resolved
mne/channels/montage.py Show resolved Hide resolved
@larsoner
Copy link
Member

Okay I had a quick chat with Alex about this. I'm back on board with most of this stuff :)

But I think I managed to convince @agramfort about a couple of things:

  1. Don't store dev_head_t. Instead, store 2 versions of the HPI points in the Digitization (soon to be list of dict): 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.

  2. Similarly, when reading dig with soon-to-be mne.channels.read_polhemus for example, just read, don't covert coord frames (don't even make it an option). The dig points will probably be in FIFFV_COORD_UNKNOWN and maybe there will be a few HPI in FIFF_COORD_HEAD. Then:

    1. Add DigMontage.transform_to_head() which will convert to head coord frame using LPA/Nasion/RPA (error if not present).
    2. In raw.set_montage we say that this will always be done because channels should always have positions in head coord frame.

Make sense?

@larsoner
Copy link
Member

Okay let's get this in once the CIs are happy and iterate on making things more general as outlined above in subsequent PRs.

@massich
Copy link
Contributor Author

massich commented Aug 22, 2019

@larsoner yes some of the things you said where poping while doing this refactoring, and this PR was just to manage to split the codebase. Hopefully the rest of the PRs are way shorter. Easy to review so that if ever anything goes wrong we would be able to trace what happened.

@agramfort
Copy link
Member

Great lets merge when green

@massich
Copy link
Contributor Author

massich commented Aug 22, 2019

updating #6461, now while waiting for Travis + clearing my thoughts.

@massich massich mentioned this pull request Aug 22, 2019
19 tasks
@agramfort agramfort merged commit 0600f81 into mne-tools:master Aug 22, 2019
@agramfort
Copy link
Member

One done !

@massich
Copy link
Contributor Author

massich commented Aug 22, 2019

Ok !!! thanks everyone for the Pull.

@agramfort, @larsoner I've updated #6461 take a look at it.

@massich massich deleted the change_dig_montage_representation branch August 22, 2019 18:13
alexrockhill pushed a commit to alexrockhill/mne-python that referenced this pull request Oct 1, 2019
…6639)

* WIP: make transformations private [run CIs]

* cosmit and notes

* WIP: move transforms 

This commit corresponds to this PR:
massich#27

* WIP: move the transformations logic into free helper functions (mne-tools#28)

* WIP: Add dig representation

* WIP: Remove transforms from  __init__

See massich#29 for details

* FIX dictionary unpacking

* fix rebase

* WIP: Keep dig and chnames only 

from massich#30

* fix 052193c

* use deprecated property

* wip: make point_names private + property

* wip: remove deprecation warnings

* wip: add ch_names attribute

* wip: do not use point_names in __repr__

* wip: remove _get_dig()

* wip with alex!!!!

* wip: fix wip

* fix pep

* xxxx:

* wip: deprecate compute_dev_head_t

* XXX: update XXX comments

* wip: add meaningful deprecation message

* wip: use new DigMontage constructor

* WIP-TST: deprecated DigMontage contruction

* TST: use atol for dev_head_t testing

* WIP: ADD make_dig_montage (and make it great again)

* wip

* fix tests

* fix test?

* don't copy

* cleanup

* more fixes

* fix

* fix fieldtrip

* FIX: Link

* update whatsnew and trigger CIs

* use master whatsnew

* update the new whatsnew
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.

4 participants