Skip to content

Conversation

@larsoner
Copy link
Member

@larsoner larsoner commented Apr 17, 2018

I think we have some code path cruft that has built up in mne/viz/topomap.py.

  1. image_mask is never used to modify the plots, so I removed it.
  2. _make_image_mask both set up the (unused) image_mask and modified pos based on autoshrink, so I renamed it to _autoshrink.
  3. outlines gets set by _check_outlines to always be a dict. So I removed the conditionals having to do with if outlines is None that happen afterward.
  4. Moves _plot_corrmap to mne/viz/topomap.py to keep plotting code in mne/viz

This is a step toward #5085. Simplifying this first and getting feedback should make the subsequent steps easier.

@larsoner larsoner changed the title FIX: Remove unused argument and refactor MRG: Remove unused argument and refactor Apr 17, 2018
@larsoner
Copy link
Member Author

@jona-sassenhagen do you have time to take a look?

@codecov
Copy link

codecov bot commented Apr 17, 2018

Codecov Report

Merging #5140 into master will increase coverage by 0.02%.
The diff coverage is 82.08%.

@@            Coverage Diff             @@
##           master    #5140      +/-   ##
==========================================
+ Coverage   88.14%   88.17%   +0.02%     
==========================================
  Files         357      358       +1     
  Lines       65777    65902     +125     
  Branches    11192    11208      +16     
==========================================
+ Hits        57982    58111     +129     
+ Misses       4958     4956       -2     
+ Partials     2837     2835       -2

vmin_, vmax_ = _setup_vmin_vmax(data_, None, None)
plot_topomap(data_.flatten(), pos, vmin=vmin_, vmax=vmax_,
res=64, axes=ax, cmap=cmap, outlines=outlines,
contours=contours, show=False, image_interp='bilinear')[0]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is verbatim copy-paste except for the removal of image_mask=None here

@jona-sassenhagen
Copy link
Contributor

So it seems it was originally used for this:

    if _is_default_outlines and image_mask is None:
        # prepare masking
        image_mask, pos = _make_image_mask(outlines, pos, res)

    if image_mask is not None and not _is_default_outlines:
        Zi[~image_mask] = np.nan

Hm.

I guess I approve.

@larsoner
Copy link
Member Author

Ahh. If you are using the default (_is_default_outlines), then you won't ever enter the second conditional. The only way you do is if you aren't using the default outlines, and you have supplied an image_mask. This has to be the same size as Zi. This does not seem easy to achieve for the user... I think it's okay, then.

@massich
Copy link
Contributor

massich commented Apr 18, 2018

LGTM

@larsoner larsoner merged commit 248fe7e into mne-tools:master Apr 18, 2018
@larsoner larsoner deleted the topomap branch April 18, 2018 13:21
britta-wstnr pushed a commit to britta-wstnr/mne-python that referenced this pull request Jul 5, 2018
* FIX: Remove unused argument and refactor

* FIX: Move _plot_corrmap
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