Fix predict in cropped mode, changed behavior of predict_proba

[sliwy]

Resolves #157

As reported in #135 we had a bug in cropped decoding mode, so we did not return values that corresponded to prediction output of our models. Averaging over crops was missing before returning predictions so predict returned argmax over wrong axis.

I've found two possible options to fix that:

1. Keep predict_proba return raw predictions (n_examples x n_classes x n_crops) and give users possibility to aggregate crops by themselves. Do averaging over crops in predict method.
2. Make predict_proba return average of all crops (n_examples x n_classes). This has at least correct size when interpreting as probabilities (we don't assure that those are valid probabilities, just output of torch Module and may need some nonlinearity at the end). If a user would like to use predictions per crop than he/she needs to use skorch forward method. I assume that this is for more advanced users.

I decided to use second option as it sounds to me more user-friendly.

I added tests for EEGClassifier and EEGRegressor checking if predicted values are correct (there are some parts repeated in both tests (like MockModule, MockDataset), are you ok with that in tests or do you prefer to create a separated module for that?).

[robintibor]

Looks good to me! I think the duplication for mockstuff is maybe still acceptable, definitely if we need again should move to separate file.
@gemeinl can you maybe briefly check if what @sliwy wrote is fine for you as well? Then I would merge already.

[robintibor]

Ah one thing why did you name it "test_unit_eeg_classifier.py" should be "test_eeg_classifier.py"

[robintibor]

In case it was because of pytest issue, fix in that way: pytest-dev/pytest#3151 (comment)

[sliwy]

@robintibor yes, it was because of that pytest issue; I added init.py to all folders with tests and changed tests' names

[agramfort]

to me it does not make sense to have a predict_proba for a regressor

[sliwy]

predict_proba is already implemented in skorch, so this changes the behavior in cropped mode to return averaged predictions per trial not raw predictions per crop. After the change it would be consistent with EEGClassifier predict_proba and predict behavior.

[agramfort]

I don't get it. What is predict_proba outputting for a regressor? is it a confusion with what sklearn calls decision_function?

…

[sliwy]

@agramfort I don't think that it has any connection with sklearn decision_function. It's rather a skorch thing. Our EEGRegressor inherits from skorch NeuralNetRegressor class which has predict_proba method implemented. It returns the output of the module's forward method as a numpy array, we can call it raw predictions.
As we modified EEGClassifier behavior to return aggregated values per trial in cropped mode (instead of raw) I think that it is good to have consistent behavior of predict_proba method for EEGClassifier and EEGRegressor. Then instead of returning predictions per crop for regressor and predictions per trial for classifier it is better to return predictions per trial for both classifier and regressor.

At this moment, EEGRegressor in cropped mode returns raw predictions per crop for both predict_proba and predict. In skorch regressor predict returns the same output as predict_proba, so for me, it is better to modify predict_proba to affect both methods.

[agramfort]

predict_proba for a regressor is for me evil. Maybe @thomasjpfan can shed some light on this decision in skorch?

[sliwy]

@agramfort just to clear it a little bit more, predict_proba is implemented in skorch NeuralNet which is a parent class for NeuralNetRegressor. It does not change much, predict_proba can be used in NeuralNetRegressor which is'evil' but it may explain somehow why it looks like that. Let's wait for comment of @thomasjpfan

[thomasjpfan]

predict_proba in the regressor does look strange to me and it would make sense to remove from NeuralNetRegressor. Currently NeuralNetRegressor, predict will do the same thing as predict_proba.

Maybe @BenjaminBossan would have insight into why NeuralNetRegressor defines predict_proba.

[BenjaminBossan]

There is no particular reason, really. It is present on NeuralNet and it would allow you to easily add another "predict mode" to your regressor if you choose to customize it.

In case you'd rather not have the method, you could declare it a property and raise an AttributeError. If we want to remove it in skorch, it would probably require deprecation cycle.

[agramfort]

I am not a skorch dev but exposing predict_proba for a regressor sounds really weird. At least here in braindecode I would vote to not expose this.

…

[robintibor]

So I see two options:

1. Create EEGNeuralNet superclass to solve it
2. Leave it as in this PR, live with predict_proba in regressor

I think 2. is simply more practical now what do you think @sliwy ?

[sliwy]

@robintibor I agree with you, it's simpler. If we merge it we will have the same API as skorch. Both predict and predict_proba will work properly for cropped and trialwise decoding. On the other hand, raising AttributeError is not a big problem, I can do this, but I am not really convinced if we should do that.

[agramfort]

be pragmatic but just add maybe a clear docstring and comment in the code to explain why it is like this. It can be very confusing

…

[robintibor]

yeah can you add such a comment @sliwy ? that would be great, then I would merge.

[sliwy]

done @robintibor, I added a comment with an explanation why we implement predict_proba for regressor and warning in the docstring to warn users that regressor does not return probabilities and it's better to use predict

[robintibor]

amazing. Since previous commits were fine on travis and now only acceptance tests fail again, have to assume it is another library change (skorch?), therefore merging..

thanks for your work!! :)