FIX: ICA.plot_properties wont accept new extrapolate argument of viz.…

[jscastanoc]

…plot_topomap

Reference issue

--

What does this implement/fix?

plot_topomap has included recently a "extrapolate" argument (#5754), however, the _plot_ica_topomap is not accepting this parameter. This pull request fixes this

Additional information

---

[agramfort]

@jscastanoc you have CI failure

also you are using directly _plot_ica_topomap? why not using directly the public API?

[jscastanoc]

@jscastanoc you have CI failure

also you are using directly _plot_ica_topomap? why not using directly the public API?

whoops, gonna check on that. And no, I get the error when calling ICA.plot_properties, that is the function that I am using

[codecov]

Codecov Report

Merging #6045 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6045   +/-   ##
=======================================
  Coverage   88.98%   88.98%           
=======================================
  Files         402      402           
  Lines       73168    73168           
  Branches    12156    12156           
=======================================
  Hits        65105    65105           
  Misses       5186     5186           
  Partials     2877     2877

[agramfort]

is extrapolate='box' the default so this does not change any current behavior?

[jscastanoc]

yes: https://github.com/mne-tools/mne-python/blob/master/mne/viz/topomap.py#L628

[mmagnuski]

yes, 'box' is the old behavior

[agramfort]

so we agree it's just for your personal usage that relies on a private function?

…

[jscastanoc]

so we agree it's just for your personal usage that relies on a private function?
…

I would not agree that it's for personal use: Without this addition, we could not access the new extrapolate options implemented for topo-plotting (in pull-request #5754) when plotting ICA patterns (for example when calling ICA.plot_properties). I am not calling _plot_ica_topomap directly.

[mmagnuski]

@agramfort That's good change, I think, not personal use: the extrapolate arg should be available in all topo plotting functions in my opinion. This is something we agreed on in my PR, but I was to check and add support for extrapolate in other function in a successive PR. I can take care of this here in the upcoming weekend.

[agramfort]

what I don't get is that you only change the API of _plot_ica_topomap function which is private. Am I missing something?

also @jscastanoc you should not open a PR from your master branch.

[jscastanoc]

what I don't get is that you only change the API of _plot_ica_topomap function which is private. Am I missing something?

also @jscastanoc you should not open a PR from your master branch.

the _plot_topomap implements extrapolate, which is accessible from the plot_topomap (this is the PR @mmagnuski is talking about)

However, when calling ICA.plot_properties, plot_topomap gets called via _plot_ica_topomap, ergo if _plot_ica_topomap does not implement extrapolate, this argument cannot be used in plot_topomap:

Currently:
ICA.plot_properties -> _plot_ica_topomap (doesnt implement extrapolate) -> plot_topomap (implements extrapolate but cannot be accessed since the caller _plot_ica_topomap does not implement it)

I hope I could explain myself a bit clearer now.

PS. Sorry about the PR from master, dont have much experience contributing to big projects. I'll keep that in mind for next time.

[mmagnuski]

@agramfort That is because plot_properties has topomap_args which is used to pass additional args to _plot_ica_topomap so no need to expose extrapolate in plot_properties.

[agramfort]

@jscastanoc can you push a tiny commit to restart the CIs. You have spurious failure and I cannot restart azure myself it seems

[larsoner]

@jscastanoc can you add an entry to the BUG section of whats_new.rst like:

- Fix support for supplying ``extrapolate`` via :ref:`ica.plot_properties(..., topomap_args=dict(extrapolate=...)) <mne.preprocessing.ICA.plot_properties>` by `Firstname Lastname`_

...
<and make sure later in the file there is>
.. _Firstname Lastname: https://your-preferred-link-url

[agramfort]

@jscastanoc can you rebase to make CIs happy?

[jscastanoc]

@jscastanoc can you rebase to make CIs happy?

@agramfort all set, I think :D (apparently some of the tests are failing randomly ?)

[agramfort]

@jscastanoc can you rebase again? I don't manage to make circle green despite some restarts

[jscastanoc]

@agramfort could you please give me a hint on this? I have no idea why that test is failing

[mmagnuski]

Circle falls over when building docs (make html), the error is:

Makefile:54: recipe for target 'html_dev-noplot' failed
make: *** [html_dev-noplot] Error 1
Exited with code 2

I don't know why this happens.

[agramfort]

can you merge current master to retrigger CIs?

[larsoner]

Look at the log, warnings become errors and there is this:

/home/circleci/project/doc/whats_new.rst:169: WARNING: undefined label: mne.preprocessing.ica.plot_properties (if the link has no caption the label must precede a section header)

I've pushed a commit to fix it

[agramfort]

thx @jscastanoc