Skip to content

MRG, ENH: Improve mixed source estimate support#7597

Merged
larsoner merged 9 commits intomne-tools:masterfrom
larsoner:stc
Apr 21, 2020
Merged

MRG, ENH: Improve mixed source estimate support#7597
larsoner merged 9 commits intomne-tools:masterfrom
larsoner:stc

Conversation

@larsoner
Copy link
Copy Markdown
Member

@larsoner larsoner commented Apr 10, 2020 •
edited
Loading

  • Redefine "mixed" source estimate as two cortical surface source spaces, plus others
  • Add MixedVectorSourceEstimate
  • Add tests for generating all source space types in inversion (mixed not tested, mixed vector not tested)
  • Add MixedVectorSourceEstimate.surface() and .volume() to allow splitting (someday later maybe we'll add putting them back together but not for now)
  • Add round-trip I/O tests for Mixed(Vector)SourceEstimate
  • Add support for MixedSourceEstimate to extract_label_time_course
  • Deprecate MixedSourceEstimate.plot_surface in favor of stc.surface().plot()

Next PRs:

In the long run:

  • Add support for Vol/MixedSourceEstimate.plot to _Brain somehow (volume rendering?)

Closes #7564

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 10, 2020

cc @fleurgaudfernau I plan to work on this in the coming days/weeks, let me know if the API plan makes sense to you

@larsoner larsoner mentioned this pull request Apr 10, 2020
Closed
@fleurgaudfernau
Copy link
Copy Markdown

fleurgaudfernau commented Apr 15, 2020

@larsoner Yes, it seems good to me!

@larsoner larsoner force-pushed the stc branch 3 times, most recently from 654abd0 to fa324c4 Compare April 17, 2020 17:58
@larsoner larsoner changed the title WIP, ENH: Improve mixed source estimate support MRG, ENH: Improve mixed source estimate support Apr 17, 2020
@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 17, 2020

@agramfort this is ready for review/merge from my end. I've updated the top description to say what will be done in subsequent PRs to make reviewing hopefully easier.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 17, 2020 •
edited
Loading

Codecov Report

Merging #7597 into master will decrease coverage by 0.02%.
The diff coverage is 87.10%.

@@            Coverage Diff             @@
##           master    #7597      +/-   ##
==========================================
- Coverage   90.17%   90.14%   -0.03%     
==========================================
  Files         454      454              
  Lines       82751    83929    +1178     
  Branches    13230    13324      +94     
==========================================
+ Hits        74621    75661    +1040     
- Misses       5266     5406     +140     
+ Partials     2864     2862       -2

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Apr 18, 2020

@larsoner can you explain the new logic of vertices_list and _vertices attributes?

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 18, 2020

In master SourceEstimate.vertices and MixedSourceEstimate.vertices are lists, whereas VolSourceEstimate.vertices often a ndarray. Not only is this inconsistent, it's problematic because you can have volume source spaces of length > 1 (when volume_label=[...] in setup_volume_source_space), and then it's a list. So the fix to me is that VolSourceEstimate.vertices should always be a list. To get it to this state, accessing VolSourceEstimate.vertices is deprecated for one cycle and people should use .vertices_list. After that .vertices and .vertices_list will be the same.

That's the plan I hatched in any case...

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 18, 2020

A (much) simpler thing to do is to make VolSourceEstimate.vertices a list with no deprecation cycle, but I'm not sure the extent to which that would break people's code...

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 18, 2020

(it could qualify as a bug really...)

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Apr 18, 2020

as long as it does not break code when reading an old file from disk I think we can consider this a bug fix.

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 18, 2020

@agramfort done

@larsoner larsoner force-pushed the stc branch 3 times, most recently from 9bd48b0 to 3dcc47f Compare April 20, 2020 14:03
@larsoner larsoner mentioned this pull request Apr 20, 2020
Merged
5 tasks
agramfort
agramfort approved these changes Apr 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

besides some cosmits LGTM ! thx @larsoner

mne/morph.py
vals['sdr_morph'].__dict__ = morph
# Backward compat with when it used to be a list
if isinstance(vals['vertices_to'], np.ndarray):
vals['vertices_to'] = [vals['vertices_to']]
Copy link
Copy Markdown
Member

@agramfort agramfort Apr 20, 2020

Choose a reason for hiding this comment

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

do we still need this?

Copy link
Copy Markdown
Member Author

@larsoner larsoner Apr 20, 2020

Choose a reason for hiding this comment

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

Yes anyone with a SourceMorph saved to disk will need this shim

mne/utils/tests/test_check.py
assert sug == '' # nothing
sug = _suggest('Left-cerebellum', names)
assert sug == " Did you mean 'Left-Cerebellum-Cortex'?"
sug = _suggest('Cerebellum-Cortex', names)
Copy link
Copy Markdown
Member

@agramfort agramfort Apr 20, 2020

Choose a reason for hiding this comment

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

nice !

@larsoner larsoner merged commit dd2a1f4 into mne-tools:master Apr 21, 2020
@larsoner larsoner deleted the stc branch April 21, 2020 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agramfort agramfort agramfort approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

issue with volumetric source estimate morphing

3 participants

@larsoner @fleurgaudfernau @agramfort