[MRG] Predict trials #312

gemeinl · 2021-07-26T13:11:40Z

I am not sure I fully understood the issue. Is this what you had in mind @robintibor ?
What would be the best location for predict_trials?

robintibor · 2021-07-27T11:22:13Z

braindecode/classifier.py

@@ -268,3 +270,17 @@ def predict(self, X):

        """
        return self.predict_proba(X).argmax(1)
+
+    def predict_trials(self, X):
+        """Return trialwise predictions and targets.


Great, can you add one sentence how the expected output shape will be for trial_predictions and trial_labels, including meaning of dimensions for trial_predictions. That should be helpful

robintibor · 2021-07-27T11:24:03Z

braindecode/classifier.py

+        -------
+        trial_predictions, trial_labels: tuple(np.ndarray, np.ndarray)
+        """
+        return predict_trials(self.module, X)


Maybe also check if self.cropped is True and if not raise error?

Well, should there be an error? We always have trials no matter if we do cropped decoding or not.

I think
a) Right now code would fail without cropped decoding? There are a lot of assumptions inside coming from cropped decoding no? Or would it just run?
b) for trialwise decoding existing predict method would already give you trialwise predictions right?

If that is correct either we assert we are in cropped mode (and maybe also add to name like predict_trials_from_cropped) or we just call regular predict function if not in cropped mode? We should check what happens atm when this function is called e.g. at end of trialwise decoding example

I am not so much familiar with the internals of the cropped decoding to know about the assumptions.
In my opinion predict_trials should always work, no matter if cropped or trialwise decoding, since we always have trials.
For trialwise decoding the output will then just be the same as calling predict. For cropped decoding it will be different.

robintibor · 2021-07-27T11:26:03Z

braindecode/regressor.py

+        -------
+        trial_predictions, trial_labels: tuple(np.ndarray, np.ndarray)
+        """
+        return predict_trials(self.module, X)


same here for comments above... Maybe we could also consider to have a superclass EEGNeuralNet for both to avoid duplication?

Yes, definitely. I just added one more duplicate to the large list of code duplicates in those classes...

robintibor · 2021-07-27T11:27:38Z

Thanks, great, yes that is what I had in mind. I was thinking if function name should somehow indicate that trial labels are being returned as well? Otherwise might be surprising? Or maybe just make return_labels:bool a parameter of the function? to return labels or not? Even with default true, would more explicitly indicate what is happening.

And of course needs tests + whats_new

gemeinl · 2021-07-27T13:13:46Z

Yes it is surprising but what you requested.
How about we rename to get_trial_preds_and_labels?
Or do you like the return_labels flag better?

robintibor · 2021-07-29T09:00:39Z

I think return_labels flag is better, predict_trials is quite nice name

robintibor · 2021-07-29T23:06:29Z

docs/whats_new.rst

@@ -30,6 +30,7 @@ Enhancements
 - Adding Mixup augmentation :class:`braindecode.augmentation.Mixup` (:gh:`254` by `Simon Brandt`_)
 - Adding saving of preprocessing and windowing choices in :func:`braindecode.preprocessing.preprocess`, :func:`braindecode.preprocessing.create_windows_from_events` and :func:`braindecode.preprocessing.create_fixed_length_windows` to datasets to facilitate reproducibility (:gh:`287` by `Lukas Gemein`_)
 - Adding :func:`braindecode.models.util.aggregate_probas` to perform self-ensembling of predictions with sequence-to-sequence models (:gh:`294` by `Hubert Banville`_)
+- Adding :func:`braindecode.training.scoring.predict_trials` to generate trialwise predictions (:gh:`312` by `Lukas Gemein`_)


Suggested change

- Adding :func:`braindecode.training.scoring.predict_trials` to generate trialwise predictions (:gh:`312` by `Lukas Gemein`_)

- Adding :func:`braindecode.training.scoring.predict_trials` to generate trialwise predictions after cropped training (:gh:`312` by `Lukas Gemein`_)

To make even clearer

I sitll disagree. For me this is not specific for cropped decoding. See comment above.

@robintibor In braindecode.training.scoring.predict_trials we cannot know whether the model was trained in cropped fashion. We need an EEGClassifier / EEGRegressor for this. Is it save to add this function anyways? Or should we remove it and only have EEGClassifier/EEGRegressor.predict_trials() which will then check self.cropped?

What about this here @robintibor

I think we should have it so people can use this if they are not using skorch.

agramfort · 2021-07-30T07:51:44Z

braindecode/regressor.py

+
+        Returns
+        -------
+        trial_predictions, trial_labels: tuple(np.ndarray, np.ndarray)


please specify the dimension of each array. This syntax tuple(np.ndarray, np.ndarray) is odd to me.

Dimension are at the top of the docstring https://github.com/braindecode/braindecode/pull/312/files/0c3e6db742d0012eac23e35ae1ba47f74dda7c88#diff-049a3b69cc29c2439db003260fe1ca5f9908afd5f4de2498c5ff7d155cf96f60R257-R258. Should I move it to the Returns section or add it here again?

yes. I won't expect dimensions in the header line of a docstring. It should be in the Returns statement.

you should skim through https://www.python.org/dev/peps/pep-0257/

we should activate https://github.com/PyCQA/pydocstyle on the repo....

I see, thank you. I support the idea for more automated checks to ensure we follow conventions.

agramfort · 2021-07-30T07:52:22Z

braindecode/training/scoring.py

@@ -295,3 +297,47 @@ def on_epoch_end(self, net, dataset_train, dataset_valid, **kwargs):
                cached_net, dataset_train, self.y_trues_
            )
        self._record_score(net.history, current_score)
+
+
+def predict_trials(module, dataset, return_targets=True):


why adding 2 ways of doing the same thing ie the method and this public function?

I guess braindecode.training.scoring.predict_trials exists because you don't need an EEGClassifier / EEGRegressor to make predictions. It is sufficient to have any kind of model given as PyTorch module as well as a braindecode dataset.

However, you would expect your estimator to provide predict.. right?

agramfort · 2021-07-30T07:54:00Z

braindecode/training/scoring.py

+
+def predict_trials(module, dataset, return_targets=True):
+    """Create trialswise predictions (n_trials x n_classes x n_predictions),
+    and optionally also return trialwise labels (n_trials x n_targets) from


n_targets and n_predictions are conceptually different things? sorry but I get confused by n_trials, something I I would call n_crops etc.

n_predictions is in time domain and depends on window_size and size of the receptive field of the net.
n_targets can be very different. It can be a single value in classification / regression tasks, it can be multiple values in multiple discrete target classification as introduced with #267, and it can also be a sequence as in #261.

Why do you get confused by n_trials? That is the point, we do not have crops / compute windows at this point. These are actual trials. If you want to generate crop / compute window predictions you would call .predict() instead. In predict_trials we invert the creation of compute windows (removing potentially overlapping predictions) to obtain trial predictions.

ok much clearer 🙏 . Maybe it's worth adding a glossary as we do with MNE https://mne.tools/stable/glossary.html ?

gemeinl · 2021-07-30T15:10:27Z

why test_variable_length_trials_decoding fails?

codecov · 2021-07-30T15:35:00Z

Codecov Report

Merging #312 (8855c7a) into master (a436f25) will increase coverage by 0.11%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master     #312      +/-   ##
==========================================
+ Coverage   80.27%   80.38%   +0.11%     
==========================================
  Files          49       49              
  Lines        3047     3085      +38     
==========================================
+ Hits         2446     2480      +34     
- Misses        601      605       +4

robintibor · 2021-07-30T15:51:58Z

Worked on Rerun, so probably just need to increase tolerance even further

gemeinl · 2021-08-09T11:40:48Z

Done from my side unless @robintibor disagrees with current implementation regarding cropped / trialwise stuff...

robintibor · 2021-08-09T12:58:55Z

braindecode/training/scoring.py

+    cropped_data = sum(dataset.get_metadata()['i_window_in_trial'] != 0) > 0
+    if not cropped_data:
+        raise ValueError('This function was designed to predict trials from '
+                         'cropped datasets. This is a trialwise dataset.')


Suggested change

cropped_data = sum(dataset.get_metadata()['i_window_in_trial'] != 0) > 0

if not cropped_data:

raise ValueError('This function was designed to predict trials from '

'cropped datasets. This is a trialwise dataset.')

more_than_one_window = sum(dataset.get_metadata()['i_window_in_trial'] != 0) > 0

if not more_than_one_window:

warnings.warn('This function was designed to predict trials from '

'cropped datasets, which typically have multiple compute windows per trial .'

'The given dataset has exactly one window per trial,')

Is it sure that this must be the case? I think this is not necessarily true. Maybe at least downgrade to a warning instead of an error?

I could not come up with a case where it does not hold. Isn't it the definition of trialwise decoding to have one window per trial?

one can consider (and at least that's what I discussed with Tonio as well as far as I recall) trialwise decoding implies single window and single prediction per trial. Whereas if you have single window but multiple predictions, than you can still consider it cropped decoding. and all the existing cropped decoding code should also run fine

Ok. Then I will update according to your suggestions

robintibor · 2021-08-09T12:59:20Z

Made one comment, what do you think @gemeinl ?

-added return_targets flag -added test -added changes to whats new -added predict_trials to api

-updated test -now raising errors and warnings if used incorrectly

robintibor · 2021-08-09T17:16:50Z

Great stuff, merged now.

gemeinl linked an issue Jul 26, 2021 that may be closed by this pull request

Utility functions for computing trial predictions after cropped training #302

Closed

gemeinl changed the title ~~Predict trials~~ [WIP] Predict trials Jul 26, 2021

robintibor reviewed Jul 27, 2021

View reviewed changes

robintibor reviewed Jul 29, 2021

View reviewed changes

agramfort reviewed Jul 30, 2021

View reviewed changes

gemeinl force-pushed the predict_trials branch from b793b1d to 70ae197 Compare July 30, 2021 14:59

gemeinl changed the title ~~[WIP] Predict trials~~ [MRG] Predict trials Aug 9, 2021

robintibor reviewed Aug 9, 2021

View reviewed changes

gemeinl added 7 commits August 9, 2021 16:57

added predict_trials to scoring, classifier, and regressor

78dffae

updated docstrings

2f65ff8

added return value to docstring

a1ad273

-improved docstrings of predict_trials

84149c5

-added return_targets flag -added test -added changes to whats new -added predict_trials to api

-improved docstrings

030f922

-updated test -now raising errors and warnings if used incorrectly

added more tests to improve codecov

ee9ed03

updating according to suggestions

1bc61a1

gemeinl force-pushed the predict_trials branch from 1f0c4e1 to 1bc61a1 Compare August 9, 2021 15:01

updated test

e7dde06

gemeinl mentioned this pull request Aug 9, 2021

Remove duplicates in EEGClassifier and EEGRegressor #320

Open

Merge branch 'master' into predict_trials

8855c7a

robintibor merged commit 5b826c4 into braindecode:master Aug 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Predict trials #312

[MRG] Predict trials #312

gemeinl commented Jul 26, 2021

robintibor Jul 27, 2021

robintibor Jul 27, 2021

gemeinl Jul 27, 2021

robintibor Jul 29, 2021

gemeinl Jul 30, 2021

robintibor Jul 27, 2021

gemeinl Jul 27, 2021

robintibor commented Jul 27, 2021

gemeinl commented Jul 27, 2021

robintibor commented Jul 29, 2021

robintibor Jul 29, 2021

gemeinl Jul 30, 2021

gemeinl Aug 9, 2021

gemeinl Aug 9, 2021

robintibor Aug 9, 2021

agramfort Jul 30, 2021

gemeinl Jul 30, 2021

agramfort Jul 30, 2021

gemeinl Jul 30, 2021

agramfort Jul 30, 2021

gemeinl Jul 30, 2021

agramfort Jul 30, 2021

gemeinl Jul 30, 2021

agramfort Jul 30, 2021

gemeinl commented Jul 30, 2021 •

edited

Loading

codecov bot commented Jul 30, 2021 •

edited

Loading

robintibor commented Jul 30, 2021

gemeinl commented Aug 9, 2021

robintibor Aug 9, 2021

gemeinl Aug 9, 2021

robintibor Aug 9, 2021 •

edited

Loading

gemeinl Aug 9, 2021

robintibor commented Aug 9, 2021

robintibor commented Aug 9, 2021

	- Adding :func:`braindecode.training.scoring.predict_trials` to generate trialwise predictions (:gh:`312` by `Lukas Gemein`_)
	- Adding :func:`braindecode.training.scoring.predict_trials` to generate trialwise predictions after cropped training (:gh:`312` by `Lukas Gemein`_)

-    cropped_data = sum(dataset.get_metadata()['i_window_in_trial'] != 0) > 0
-    if not cropped_data:
-        raise ValueError('This function was designed to predict trials from '
-                         'cropped datasets. This is a trialwise dataset.')
+    more_than_one_window = sum(dataset.get_metadata()['i_window_in_trial'] != 0) > 0
+    if not more_than_one_window:
+        warnings.warn('This function was designed to predict trials from '
+            'cropped datasets, which typically have multiple compute windows per trial .'
+            'The given dataset has exactly one window per trial,')

[MRG] Predict trials #312

[MRG] Predict trials #312

Conversation

gemeinl commented Jul 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robintibor commented Jul 27, 2021

gemeinl commented Jul 27, 2021

robintibor commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gemeinl commented Jul 30, 2021 • edited Loading

codecov bot commented Jul 30, 2021 • edited Loading

Codecov Report

robintibor commented Jul 30, 2021

gemeinl commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robintibor Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robintibor commented Aug 9, 2021

robintibor commented Aug 9, 2021

gemeinl commented Jul 30, 2021 •

edited

Loading

codecov bot commented Jul 30, 2021 •

edited

Loading

robintibor Aug 9, 2021 •

edited

Loading