ENH: add corrmap. by dengemann · Pull Request #1985 · mne-tools/mne-python

dengemann · 2015-04-20T01:29:04Z

Taking over #1795

larsoner · 2015-04-20T02:26:20Z

mne/preprocessing/ica.py

Is there a more elegant way to do this? a problem with try/except over a bunch of lines is that sometimes you end up hitting an unintended error. It's also not clear from the code where or why an IndexError would occur, so it's also harder for the next person to deal with.

I know ... tried to brainwash @jona-sassenhagen but he was very resistant so far.

I understand the "it's easier to ask for forgiveness then for permission" mentality, but it's not clear in this code what the forgiveness would even be for, which makes it difficult to understand. I think it's typically applied to a single or a couple lines of code. Looking at these ~10 lines of code deeper I still can't figure out where to expect an indexing error, which is not a good sign.

I agree. let's figure it out :)

larsoner · 2015-04-20T02:33:12Z

Other than my comments, LGTM, but I don't really know much about the ICA code

dengemann · 2015-04-20T02:50:03Z

deleted comment, wrong issue.

agramfort · 2015-04-20T09:36:29Z

do you want this merged for the release?

larsoner · 2015-04-20T16:28:31Z

@agramfort #1795, the PR that this one superceded, was marked for 0.9 so I think yeah he wants it for the release

dengemann · 2015-04-20T16:29:07Z

do you want this merged for the release?

let's try to do that.

larsoner · 2015-04-22T17:01:53Z

@dengemann don't forget about this one

dengemann · 2015-04-22T17:07:26Z

I'm hesitating / thinking about moving this to 0.10. Let me see how this weeks schedule develops (unless @jona-sassenhagen want's to complete it). At this stage it's more important though to fix bugs than adding new features.

jona-sassenhagen · 2015-04-22T17:14:25Z

Is there anything major missing? I wouldn't stress it being a .9 feature, but I'd like to have it merged so the people I work with have an easier time using it :)

dengemann · 2015-04-22T17:15:52Z

Is there anything major missing? I wouldn't stress it being a .9 feature, but I'd like to have it merged so the people I work with have an easier time using it :)

Nope just addressing the comments.

move nested functions to private functions at the module level
remove try catch blocks

dengemann · 2015-04-22T17:16:17Z

and some minor points.

jona-sassenhagen · 2015-04-22T17:29:00Z

I have a bit of time now, I'll try to get at least one of the big two

jona-sassenhagen · 2015-04-22T17:29:10Z

that is, this week, not right now :)

larsoner · 2015-04-22T17:29:35Z

I'll just do it now, they're my gripes anyway :)

jona-sassenhagen · 2015-04-22T17:30:02Z

better still!

dengemann · 2015-04-22T17:32:04Z

@Eric89GXL you do have push access. Feel free to work on my PR.

larsoner · 2015-04-22T18:48:55Z

done, @dengemann and/or @jona-sassenhagen please have a look. I got rid of the try/excepts other than the one I thought made sense (the read_meas_info one is a good example of the ask for forgiveness principle IMO) by checking for failure conditions. Note that this is also a better approach because it avoided a spewing of numpy warnings in some cases before hitting the eventual ValueError.

agramfort · 2015-04-22T19:08:46Z

mne/preprocessing/ica.py

{} -> dict()

it's more explicit

coveralls · 2015-04-23T22:33:30Z

Coverage decreased (-0.0%) to 90.29% when pulling 0ca2a34 on dengemann:patch-3 into 6b08e27 on mne-tools:master.

coveralls · 2015-04-23T22:33:31Z

Coverage decreased (-0.0%) to 90.29% when pulling 0ca2a34 on dengemann:patch-3 into 6b08e27 on mne-tools:master.

coveralls · 2015-04-23T22:33:31Z

Coverage decreased (-1.83%) to 88.46% when pulling 0ca2a34 on dengemann:patch-3 into 6b08e27 on mne-tools:master.

agramfort · 2015-04-24T06:54:24Z

mne/preprocessing/ica.py

rm empty line

wmvanvliet · 2015-04-24T08:46:57Z

the current implementation breaks if there are more then 20 IC's being plotted:

/Users/rodin/BRU/mne-python/mne/preprocessing/ica.py in _plot_corrmap(data, subjs, indices, ch_type, ica, label, show, outlines, layout, cmap, contours)
   2189         figs = [_plot_corrmap(data[k:k + p], subjs[k:k + p],
   2190                 indices[k:k + p], ch_type, ica, label)
-> 2191                 for k in range(0, n_components, p)]
   2192         return figs
   2193     elif np.isscalar(picks):

TypeError: _plot_corrmap() takes exactly 11 arguments (6 given)

jona-sassenhagen · 2015-04-24T16:53:17Z

mne/preprocessing/ica.py

Label defaults to None and should stay None for dry runs, in which case we get an error here.

larsoner · 2015-04-28T04:51:55Z

@dengemann should we move this to 0.10, or do you have time to finish it?

jona-sassenhagen · 2015-04-29T11:23:03Z

Would it help if I made proposals/PRs for the problems noted by @wmvanvliet and I?

dengemann · 2015-04-29T11:52:17Z

As said before please take over if you have time.

On 29 Apr 2015, at 13:23, jona-sassenhagen notifications@github.com wrote:

Would it help if I made proposals/PRs for the problems noted by @wmvanvliet and I?

—
Reply to this email directly or view it on GitHub.

agramfort · 2015-05-12T19:51:24Z

I would move this to 0.10... but you can object...

jona-sassenhagen · 2015-05-12T19:52:49Z

@agramfort I think this is basically done.

jona-sassenhagen · 2015-05-12T19:54:01Z

I have a PR addressing the problems noted by wmvanvliet and I.

larsoner · 2015-05-12T19:54:45Z

After that is merged it will need a rebase, too

larsoner · 2015-05-12T19:55:04Z

@dengemann if you are swamped I can merge the PR into your branch and rebase

agramfort · 2015-05-12T19:59:19Z

my 0.5 concern is that I don't know if this method is what we should advertise and it's currently the best we can offer. The problem when adding code to a package is that it never goes away so we need to think twice...

dengemann · 2015-05-12T20:07:25Z

Agreed. Let's move slowly, test it extensivel, and keep it on master for
now.

2015-05-12 21:59 GMT+02:00 Alexandre Gramfort notifications@github.com:

my 0.5 concern is that I don't know if this method is what we should
advertise and it's currently the best we can offer. The problem when adding
code to a package is that it never goes away so we need to think twice...

—
Reply to this email directly or view it on GitHub
#1985 (comment)
.

jona-sassenhagen · 2015-05-12T20:08:58Z

On master instead of release? Fine with me.

@dengemann, I'm up for a massive test comparing different ICA and IC identification mechanisms on many data sets ...

agramfort · 2015-05-12T20:13:17Z

I would vote not to merge this to master before someone does an big validation like you proposed. If we merge to master it will be released in the following release

dengemann · 2015-05-12T20:14:35Z

I meant merge it to 0.10 master once it's open and once we're happy.

2015-05-12 22:13 GMT+02:00 Alexandre Gramfort notifications@github.com:

I would vote not to merge this to master before someone does an big
validation like you proposed. If we merge to master it will be released in
the following release

—
Reply to this email directly or view it on GitHub
#1985 (comment)
.

jona-sassenhagen · 2015-05-16T04:33:18Z

I've finally fixed the stuff that went broke in the meantime with updates on master (e.g. giving everything a show keyword destroyed the plotting), should I make a new PR or PR to @dengemann 's repo?..

agramfort · 2015-05-16T07:59:08Z

Make a new PR to take the lead and get some free time to @dengemann ;)

jona-sassenhagen · 2015-05-16T10:45:15Z

Close for #2104?

dengemann mentioned this pull request Apr 20, 2015

Add Corrmap #1795

Closed

larsoner reviewed Apr 20, 2015
View reviewed changes

larsoner added this to the 0.9 milestone Apr 20, 2015

larsoner force-pushed the patch-3 branch from 78a8f3d to 974e29a Compare April 22, 2015 18:46

agramfort reviewed Apr 22, 2015
View reviewed changes

mne/preprocessing/ica.py Outdated

Copy link
Copy Markdown

Member

agramfort Apr 22, 2015

Choose a reason for hiding this comment

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

{} -> dict()

it's more explicit

dengemann force-pushed the patch-3 branch from a9447aa to 0ca2a34 Compare April 23, 2015 22:01

agramfort reviewed Apr 24, 2015
View reviewed changes

mne/preprocessing/ica.py

Copy link
Copy Markdown

Member

agramfort Apr 24, 2015

Choose a reason for hiding this comment

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

rm empty line

jona-sassenhagen reviewed Apr 24, 2015
View reviewed changes

larsoner mentioned this pull request May 12, 2015

MRG: GAT small fixes #2072

Merged

dengemann modified the milestones: 0.10, 0.9 May 12, 2015

jona-sassenhagen mentioned this pull request May 16, 2015

MRG: More work on Corrmap #2104

Merged

agramfort closed this May 16, 2015

larsoner mentioned this pull request Aug 2, 2023

BUG: Fix bugs with corrmap computation #11858

Merged

Uh oh!

Conversation

dengemann commented Apr 20, 2015

Uh oh!

larsoner Apr 20, 2015

Choose a reason for hiding this comment

Uh oh!

dengemann Apr 20, 2015

Choose a reason for hiding this comment

Uh oh!

larsoner Apr 20, 2015

Choose a reason for hiding this comment

Uh oh!

dengemann Apr 20, 2015

Choose a reason for hiding this comment

Uh oh!

larsoner commented Apr 20, 2015

Uh oh!

dengemann commented Apr 20, 2015

Uh oh!

agramfort commented Apr 20, 2015 via email

Uh oh!

larsoner commented Apr 20, 2015

Uh oh!

dengemann commented Apr 20, 2015

Uh oh!

larsoner commented Apr 22, 2015

Uh oh!

dengemann commented Apr 22, 2015

Uh oh!

jona-sassenhagen commented Apr 22, 2015

Uh oh!

dengemann commented Apr 22, 2015

Uh oh!

dengemann commented Apr 22, 2015

Uh oh!

jona-sassenhagen commented Apr 22, 2015

Uh oh!

jona-sassenhagen commented Apr 22, 2015

Uh oh!

larsoner commented Apr 22, 2015

Uh oh!

jona-sassenhagen commented Apr 22, 2015

Uh oh!

dengemann commented Apr 22, 2015

Uh oh!

larsoner commented Apr 22, 2015

Uh oh!

agramfort Apr 22, 2015

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 23, 2015

Uh oh!

coveralls commented Apr 23, 2015

Uh oh!

coveralls commented Apr 23, 2015

Uh oh!

agramfort Apr 24, 2015

Choose a reason for hiding this comment

Uh oh!

wmvanvliet commented Apr 24, 2015

Uh oh!

jona-sassenhagen Apr 24, 2015

Choose a reason for hiding this comment

Uh oh!

larsoner commented Apr 28, 2015

Uh oh!

jona-sassenhagen commented Apr 29, 2015

Uh oh!

dengemann commented Apr 29, 2015

Uh oh!

agramfort commented May 12, 2015

Uh oh!

jona-sassenhagen commented May 12, 2015

Uh oh!

jona-sassenhagen commented May 12, 2015

Uh oh!

larsoner commented May 12, 2015

Uh oh!

larsoner commented May 12, 2015