@Eric89GXL We currently have keywords plot and show. Shouldn't it be only show, or am I missing the rationale behind what you did there?

IIRC someone at some point wanted the functionality without any plots being made, thus the plot option. Even doing the plots without showing them could incur some overhead. This suggests to me that the two bits of functionality should be separated (promotes orthogonality of the functions), but I'm not actually very familiar with what it does.

I guess there could be a situation where you want the figures constructed, but only plotted later, in a different context, but right now, to me it seems mostly confusing to have both keywords.

the rationale is:

• if function / method starts with plot_ then we need a show param.
• if it's a function that runs numerics and can plot something for visual
  inspection then use a plot param

ok?

I'm repeatedly coming across situations where I'd like to tell other users of MNE to use corrmap. I forgot, did we have any reservations regarding merging this once cleaned up, and which? It is after all an experimentally verified algorithm. @agramfort

No I don't think so. We can merge it. Can you rebase?

@jona-sassenhagen did the points about plotting and showing get cleared up through your changes?

please indeed rebase / clean up. thx

@Eric89GXL from what I understand, there are a bunch of changes here made by you that are seemingly unrelated to corrmap, e.g. the ones in mne/cov.py or mne/tests/test_import_nesting.py. I probably don't understand all of them. I'd like to continue working only with code I actually roughly understand the implications of, would it be possible to stash all of the changes irrelevant to corrmap?

The test_import_nesting.py changes were necessary in order to get tests to pass on this branch, so they need to stay. The changes to cov.py are pretty minor (overriding copy method) so I wouldn't worry about that one. I think if you don't touch either of those changesets you should be fine to continue working on the other bits.

There we are. I didn't really do anything but catch up with master.

Merge please? :)

@jona-sassenhagen for future reference we prefer rebasing to merging. I'll let @dengemann or @agramfort have a look and merge if they're happy.

There is 1 error in one of the Travis checks:

ERROR: Test fieldtrip_client
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/mne-tools/mne-python/mne/utils.py", line 622, in dec
    return function(*args, **kwargs)
  File "/home/travis/build/mne-tools/mne-python/mne/realtime/tests/test_fieldtrip_client.py", line 55, in test_fieldtrip_client
    tmax=5, wait_max=1) as rt_client:
  File "/home/travis/build/mne-tools/mne-python/mne/realtime/fieldtrip_client.py", line 91, in __enter__
    raise RuntimeError('Could not connect to FieldTrip Buffer')
RuntimeError: Could not connect to FieldTrip Buffer

Rest is okay. I think the error is not about the actual code?

@jona-sassenhagen you should squash all commits into one to make sure the diff is up to date and history is clean.

If you can. Or everything in one if its a pain

On 4 oct. 2015, at 15:28, Eric Larson notifications@github.com wrote:

Squash by author though so blame works, right @agramfort?
—
Reply to this email directly or view it on GitHub.

Soooo ... what exactly do I do to squash?

git reset --soft HEAD~3000
git commit -m "squash"

?

This is what Google tells me.

Ok so I create a new branch, merge in this branch, reset, merge, and then make a new PR for it?

Don't open a new PR. Just force push

On 4 oct. 2015, at 22:49, jona-sassenhagen notifications@github.com wrote:

Ok so I create a new branch, merge in this branch, reset, merge, and then make a new PR for it?

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

I think that was it? Thanks @agramfort

When I checkout 70d3895 and merge in master now, I get a merge conflict in whats_new and in mne/tests/test_utils.py. The test_utils one should be simple: the import from mne.utils in line 10-20 shoudl be

from mne.utils import (set_log_level, set_log_file, _TempDir,
                       get_config, set_config, deprecated, _fetch_file,
                       sum_squared, estimate_rank,
                       _url_to_local_path, sizeof_fmt, _check_subject,
                       _check_type_picks, object_hash, object_diff,
                       requires_good_network, run_tests_if_main, md5sum,
                       ArgvSetter, _memory_usage, check_random_state,
                       _check_mayavi_version, requires_mayavi,
                       set_memmap_min_size, _get_stim_channel, _check_fname,
                       create_slices, _time_mask, random_permutation,
                       get_call_line, verbose, compute_corr)

ica.py is merged in without problems.

Ohhh, it's not just that one commit, it's that commit and the ones that precede it. I thought you had squashed it all down to one.

Sorry, no. That's what I mean - everything up to 70d3895.

@jona-sassenhagen I have the new version up on my fork Eric89GXL under the branch corrmap. If you do something like this, it will update this PR:

git checkout patch-8
git fetch Eric89GXL
git reset --hard Eric89GXL/corrmap
git push origin --force patch-8

Or you can give me push access to your repo and I'll push it there.

@jona-sassenhagen pushed, we'll see if the CIs are happy. Ready to go from your end if it is?

Yup. I can't think of a use case I haven't covered. Thanks.

@agramfort or @dengemann did either of you want to look again, or okay for merge from you?

@jona-sassenhagen Travis fails because of a couple of style things, I'll push a fix

CIs are happy, +1 for merge from my end.

Nice.

thanks guys !

Thanks!

thanks @jona-sassenhagen! looking forward to using it

and @Eric89GXL :)

Amazing, finally merged. Thanks @jona-sassenhagen !