restored previous tests behavior

[sliwy]

Instead of performing any computations in test we always return mocked values of self.preds

[sliwy]

This fixes all the tests failing in test_eegneuralnet.py

When it comes to test_scoring.py and test_post_epoch_train_scoring it fails because we do not provide y in this line:
clf.fit(train_set, y=None, epochs=4)
and we use torchDataset but not BaseConcatDataset and not WindowsDataset so signal_kwargs['n_outputs'] is not set anywhere. We would need another elif for just plain torchDataset in _set_signal_args? what do you think?

[PierreGtch]

@sliwy There is a case for torchDataset in _set_signal_args but we have no assumption on the way y is provided. Maybe we should not support torchDataset at all, only WindowsDataset, BaseConcatDataset and np array (and later np.Epochs?)

[sliwy]

how about assumption that the second item returned by __getitem__ is the target to predict?

[PierreGtch]

But we can not load the whole dataset just to get all possible values for y. (loading the first item only is fine to get the shape of X IMHO)

[sliwy]

yeah that's true

[PierreGtch]

Maybe we can just make two mock model classes? one for the old tests that returns pred and one for the new ones that can be trained

[sliwy]

I tried to provide y explicitly in the fit call but it doesn't work, same error, should it be like that?

clf.fit(train_set, y=y, epochs=4)

[sliwy]

maybe we should use y if it is provided? otherwise it is useless?

second thing, if we do not support plain torchDataset then we should throw an error other than n_outputs not defined

[PierreGtch]

I don’t think we should use y if X is a dataset because y will not be used for training anyway so it could be confusing for the users. y should either be completely ignored or completely used, not half

[PierreGtch]

second thing, if we do not support plain torchDataset then we should throw an error other than n_outputs not defined

My philosophy was to fill all the parameters I can and ignore the others

[sliwy]

philosophy understood 😃 , there could be code that worked with plain torchDataset which was the case even in our tests and now it breaks with an error that doesn't really say what happened

[sliwy]

that' what I'm worrying about but maybe it's not so important

[PierreGtch]

I don’t think it should be the case because currently all examples provide n_outputs or the old n_classes to the model

[PierreGtch]

Are you ok to do 2 different mock model classes in the test?

[sliwy]

I'm trying to provide X and y separately, so we do not use EEGDataset

[sliwy]

and for some reason I'm receiving a strange error that was not there before:

RuntimeError: Calculated padded input size per channel: (1 x 1). Kernel size: (30 x 1). Kernel size can't be greater than actual input size

but there is no layer that has kernel size of 30 😅

[sliwy]

@PierreGtch do you have any idea why it's like that:

model output:

ShallowFBCSPNet(
  (ensuredims): Ensure4d()
  (dimshuffle): Rearrange('batch C T 1 -> batch 1 T C')
  (conv_time_spat): CombinedConv(
    (conv_time): Conv2d(1, 40, kernel_size=(25, 1), stride=(1, 1))
    (conv_spat): Conv2d(40, 40, kernel_size=(1, 3), stride=(1, 1), bias=False)
  )
  (bnorm): BatchNorm2d(40, eps=1e-05, momentum=0.1, affine=True, track_running_stats=True)
  (conv_nonlin_exp): Expression(expression=square) 
  (pool): AvgPool2d(kernel_size=(2, 1), stride=(1, 1), padding=0)
  (pool_nonlin_exp): Expression(expression=safe_log) 
  (drop): Dropout(p=0.5, inplace=False)
  (conv_classifier): Conv2d(40, 2, kernel_size=(75, 1), stride=(1, 1))
  (softmax): LogSoftmax(dim=1)
  (squeeze): Expression(expression=squeeze_final_output) 
)

clf.module_ output after training:

ShallowFBCSPNet(
  (ensuredims): Ensure4d()
  (dimshuffle): Rearrange('batch C T 1 -> batch 1 T C')
  (conv_time_spat): CombinedConv(
    (conv_time): Conv2d(1, 40, kernel_size=(25, 1), stride=(1, 1))
    (conv_spat): Conv2d(40, 40, kernel_size=(1, 3), stride=(1, 1), bias=False)
  )
  (bnorm): BatchNorm2d(40, eps=1e-05, momentum=0.1, affine=True, track_running_stats=True)
  (conv_nonlin_exp): Expression(expression=square) 
  (pool): AvgPool2d(kernel_size=(75, 1), stride=(15, 1), padding=0)
  (pool_nonlin_exp): Expression(expression=safe_log) 
  (drop): Dropout(p=0.5, inplace=False)
  (conv_classifier): Conv2d(40, 2, kernel_size=(30, 1), stride=(1, 1))
  (softmax): LogSoftmax(dim=1)
  (squeeze): Expression(expression=squeeze_final_output) 
)

it looks like during training the conv_classifier and pool layers are changed to default params? no clue for now just sharing here, maybe you have any idea

[PierreGtch]

If the model was passed initialised to the EEGClassifier, then it was probably re-initialized. I think we should just raise an issue if an initialised model is passed to EEGClassifier or EEGRegressor

[sliwy]

To me it's not the way we should go, at least not without deprecation and raising errors when nn.Module is provided. There is no example that use class as parameter to EEGClassifier, so it is breaking API in a way that I would not expect.

But maybe we can fix that in the code - Why do we filter the kwargs only to signal_kwargs? If we decide to use them we should use them for everything, otherwise I don't think it's a good change.

[sliwy]

okey, I see, I used inspect.signature only one or two times, so now I see it may not be possible :/ but I still don't feel comfortable with changing so much the API, it requires more discussion, but maybe you had discussed that during the sprint

[PierreGtch]

Passing initialised models to skorch is not a good practice and not recommended: https://skorch.readthedocs.io/en/stable/user/neuralnet.html#module

I would be in favour of not supporting initialised models and raising an error.

We did not discuss that during the sprint. @bruAristimunha, @robintibor what do you think?

[sliwy]

I agree! it's just that I think we should take this decision consciously and not by the way :) I think after changes it will be easier to use it with skorch and scikit-learn API overall.

However, we will need some additional changes

1. Model should be created at __init__ with proper config, e.g., to_dense_prediction_model called at __init__ or in _EEGNeuralNet.
2. There won't be any easy operations performed before, like running get_output_shape in cropped decoding to know how many outputs there are for one window (we may create the model object and perform everything before creating EEGClassifier but it may be not so convenient. After we have cropped decoding refactored it may be easier?
3. This code should at least for one version provide support for nn.Module as a parameter. Just for our users, so we do not break the API just like that. I propose to not use skorch kwargs and create our signal_kwargs and override the NeuralNet.initialized_instance to use our signal_kwargs additionally. Here, we should agree on behavior.
4. Checks that someone is not doing wrong thing - passing nn.Module because it silently restore default parameters to the network (in the worst case without any error), so you can, for example, perform hyperparams search with always default params of the module used for training while you provided a set of params while initializing the model.

Additional point:
5. I would move (or just expose) almost all methods from EEGModuleMixin to _EEGNeuralNet. After the changes almost nobody will use EEGModuleMixin except at model creation inside _EEGNeuralNet because EEGModuleMixin will be created only in skorch internally. Then I would prefer to have _EEGNeuralNet expose or inherit from EEGModuleMixin. :)