MRG: fill_value in ReceptiveField #4543

larsoner · 2017-09-08T22:55:21Z

This code:

Fixes some fill_value logic for the delaying
Cleans up the logic for delaying
Fixes / improves the logic for Laplacian, simultaneously making it easier in the future to do Laplacian of arbitrary neighbors (instead of just grid_to_graph)
Fixes a bug with TDR having to do with edge effects in the autocorrelation matrix terms.

larsoner · 2017-09-08T23:37:19Z

I realize now why this "bug" never showed up. We have # X is now shape (n_times, n_epochs, n_feats, n_delays) when using _delay_and_reshape in the code ReceptiveField code. So it won't really be exposed by using ReceptiveField.

In any case, I need to still fix the TDR bug.

larsoner · 2017-09-09T02:29:09Z

Found and fixed a bug in the Laplacian calculations and updated the tutorial. Also made it easier if, in the future, we want to supply an arbitrary connectivity/adjacency matrix (instead of assuming grid_to_graph-like structure like we do now)

larsoner · 2017-09-09T02:38:52Z

Updated tutorial here (yes I know I need to fix the xlim in the STRF plots and the text placement in the bottom panel of the last figure)

agramfort · 2017-09-09T10:07:54Z

+1 to fix xlim

codecov-io · 2017-09-09T14:42:53Z

Codecov Report

Merging #4543 into master will decrease coverage by 0.21%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master    #4543      +/-   ##
==========================================
- Coverage    80.9%   80.68%   -0.22%     
==========================================
  Files         349      349              
  Lines       65441    65568     +127     
  Branches    10158    10171      +13     
==========================================
- Hits        52945    52904      -41     
- Misses       9566     9709     +143     
- Partials     2930     2955      +25

larsoner · 2017-09-11T18:37:27Z

Okay this is ready for review/merge from my end. Should have it for 0.15 as it contains a couple small but important bug fixes.

agramfort · 2017-09-11T18:53:47Z

+1 for MRG when CIs are green. Thx a lot @larsoner for hunting these bugs !

larsoner · 2017-09-11T18:56:28Z

@choldgraf @nbara any interest in reviewing?

larsoner · 2017-09-11T19:09:52Z

Updated example:

https://5646-1301584-gh.circle-artifacts.com/0/tmp/circle-artifacts.dvicc8B/html/auto_tutorials/plot_receptive_field.html#sphx-glr-auto-tutorials-plot-receptive-field-py

nbara · 2017-09-11T19:20:19Z

tutorials/plot_receptive_field.py

+for ii, (rf_lap, rf, i_alpha) in enumerate(zip(models_lap, models, alphas)):
+    ax = plt.subplot2grid([3, len(alphas)], [0, ii], 1, 1)
+    ax.pcolormesh(times, rf_lap.feature_names, rf_lap.coef_[0], **kwargs)
+    ax.set(xticks=[], yticks=[])


white spaces are still visible in example, so I'm assuming this needs to be
ax.set(xticks=[], yticks=[], xlim=times[[0, -1]])

nbara · 2017-09-11T19:35:53Z

Woah this is quite the code cleanup! It's looking really nice!

I didn't spot anything else, but I'll let @choldgraf give the +1 as he would know better than me anyway.

larsoner · 2017-09-11T19:38:20Z

Glad the code is readable enough, it was a nightmare to figure out why they weren't giving equivalent results!

larsoner · 2017-09-11T19:59:24Z

Updated example with fixed bounds here

choldgraf

Whoah! This is way more complex than I was expecting. I kinda feel like you solved multiple problems in this PR other than just the mean_value issue, no? Either way, this looks like a great improvement...a few small comments

choldgraf · 2017-09-11T20:00:09Z

doc/conf.py

    'download_section_examples': False,
    'thumbnail_size': (160, 112),
-    }
+    'min_reported_time': 1.,


are we using sphinx-gallery master?

choldgraf · 2017-09-11T20:02:00Z

mne/decoding/receptive_field.py

+                                   fill_mean=self.fit_intercept)
+            X = _reshape_for_est(X)
+            # Concat times + epochs
+            if y is not None:


just confirming that you want y is not None to be inside the if statement, since before it was outside the if statement

Yep, TimeDelayingRidge is now epochs-aware

choldgraf · 2017-09-11T20:03:29Z

mne/decoding/receptive_field.py

    Parameters
    ----------
-    X : array, shape (n_times[, n_epochs], n_features)
+    X : array, shape (n_times[, n_epochs][, n_features])


I was thinking to require at the least (n_times, n_features) to avoid confusion about what the extra dimension means. E.g., if we allow for just (n_times, n_epochs) AND (n_times, n_features), how do we know which the user wants if they give a 2D input?

The code does already actually, I'll update the docstring

choldgraf · 2017-09-11T20:04:07Z

mne/decoding/receptive_field.py

+    delayed : array, shape(n_times[, n_epochs][, n_features], n_delays)
        The delayed data. It has the same shape as X, with an extra dimension
-        created at ``newaxis`` that corresponds to each delay.
+        appended to the end.


yeah this seems like a better way of doing it, I agree.

choldgraf · 2017-09-11T20:06:08Z

mne/decoding/tests/test_receptive_field.py

+    # all negative
+    smin, smax = -2, -1
+    X_del = _delay_time_series(X, smin, smax, 1.)
+    X_del.shape = (X.shape[0], -1)


Any chance you could add some comments here so that it'll be easier for somebody else to debug?

choldgraf · 2017-09-11T20:06:33Z

mne/decoding/tests/test_receptive_field.py

+    x_xt = _compute_corrs(X, np.zeros((X.shape[0], 1)), -smax, -smin + 1)[0]
+    assert_allclose(x_xt, expected)
+
+    # even worse


and, e.g., what you mean by "slightly harder" and "even worse"?

choldgraf · 2017-09-11T20:07:45Z

mne/decoding/time_delaying_ridge.py


 def _compute_corrs(X, y, smin, smax):
-    """Compute the auto- and cross-correlations."""
+    """Compute auto- and cross-correlations."""


this relates to #3884

Yep, and the changes here actually make it easier to use either pipeline interchangeably if we want to do so at some point

choldgraf · 2017-09-11T20:09:29Z

mne/decoding/time_delaying_ridge.py

        Parameters
        ----------
-        X : array, shape (n_samples, n_features)
+        X : array, shape (n_samples[, n_epochs], n_features)


there's a docstring discrepancy between here and _delay_time_series...that OK?

(referring to [n_features])

larsoner · 2017-09-11T20:21:54Z

@choldgraf comments addressed, let me know if you're happy (or just hit "approve")

choldgraf

(pending travis being happy)

agramfort · 2017-09-12T06:52:51Z

thx @larsoner

choldgraf · 2017-09-12T15:32:51Z

seriously, this was a way better PR than I would have put together :-)

larsoner added this to the 0.15 milestone Sep 8, 2017

larsoner mentioned this pull request Sep 8, 2017

mean fill bug #4521

Closed

choldgraf and others added 3 commits September 8, 2017 22:28

mean fill bug

4d9b7fb

WIP: Simplify delay code

11fc750

FIX: Fix Laplacian calculations

a32c539

larsoner force-pushed the mean_rf branch from 78e525a to a32c539 Compare September 9, 2017 02:28

larsoner changed the title ~~WIP: fill_value in ReceptiveField~~ MRG: fill_value in ReceptiveField Sep 11, 2017

ENH: Fixes for TDR and Ridge

a12c98a

larsoner force-pushed the mean_rf branch from 77c6ac2 to a12c98a Compare September 11, 2017 18:51

agramfort approved these changes Sep 11, 2017

View reviewed changes

nbara reviewed Sep 11, 2017

View reviewed changes

FIX: Minor fixes to plotting and doc building

323fbcc

choldgraf reviewed Sep 11, 2017

View reviewed changes

FIX: Better comments [ci skip]

05d3f94

choldgraf approved these changes Sep 11, 2017

View reviewed changes

agramfort merged commit cf9ff8b into mne-tools:master Sep 12, 2017

larsoner deleted the mean_rf branch September 12, 2017 13:32

nbara mentioned this pull request Sep 12, 2017

WIP: Inverse-transform ReceptiveField #4546

Merged

4 tasks

Uh oh!

MRG: fill_value in ReceptiveField #4543

MRG: fill_value in ReceptiveField #4543

Uh oh!

Conversation

larsoner commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Sep 8, 2017

Uh oh!

larsoner commented Sep 9, 2017

Uh oh!

larsoner commented Sep 9, 2017

Uh oh!

agramfort commented Sep 9, 2017

Uh oh!

codecov-io commented Sep 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

larsoner commented Sep 11, 2017

Uh oh!

agramfort commented Sep 11, 2017

Uh oh!

larsoner commented Sep 11, 2017

Uh oh!

larsoner commented Sep 11, 2017

Uh oh!

nbara Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbara commented Sep 11, 2017

Uh oh!

larsoner commented Sep 11, 2017

Uh oh!

larsoner commented Sep 11, 2017

Uh oh!

choldgraf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner commented Sep 11, 2017

Uh oh!

choldgraf left a comment

Choose a reason for hiding this comment

Uh oh!

agramfort commented Sep 12, 2017

Uh oh!

choldgraf commented Sep 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

larsoner commented Sep 8, 2017 •

edited

Loading

codecov-io commented Sep 9, 2017 •

edited

Loading

nbara Sep 11, 2017 •

edited

Loading