Automatically populate the signal-related parameters

[PierreGtch]

cf #488, #457

[PierreGtch]

@sliwy I merged the test_eeg_classifier.py and test_eeg_regressor.py tests and updated them to use a “realistic" fake model with input-related parameters.

Could you help me update the test you had in them?

[sliwy]

@PierreGtch what do you need from me? some specific test you think about?

[PierreGtch]

@sliwy I broke the tests:

• test_trialwise_predict_and_predict_proba
• test_cropped_predict_and_predict_proba
• test_cropped_predict_and_predict_proba_not_aggregate_predictions
• and test_predict_trials
  but I’m not familiar with the cropped parameter and the CroppedLoss. So not sure how to update them. Did you write them?

[sliwy]

taking a look!

[sliwy]

@PierreGtch let's move back here:
test_eegneuralnet.py is fixed in the PR: PierreGtch#3

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?

[bruAristimunha]

Hey @PierreGtch and @sliwy,

Please ping me if you need any help to solve the design conflict. To be honest, I looked the discussion at the PierreGtch#3, and I still don't get the big picture. I am available to chat or meet if necessary.

[PierreGtch]

As discussed in PierreGtch#3, we deprecate passing an initialized module to _EEGNeuralNet and skip setting signal-related arguments in this case.

[codecov]

Codecov Report

Merging #517 (0396d03) into master (997fda4) will increase coverage by 0.12%.
The diff coverage is 87.17%.

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
+ Coverage   84.35%   84.48%   +0.12%     
==========================================
  Files          63       63              
  Lines        4578     4653      +75     
==========================================
+ Hits         3862     3931      +69     
- Misses        716      722       +6     

[PierreGtch]

@sliwy are you ok to merge this version?

[PierreGtch]

Hey @PierreGtch and @sliwy,

Please ping me if you need any help to solve the design conflict. To be honest, I looked the discussion at the PierreGtch#3, and I still don't get the big picture. I am available to chat or meet if necessary.

Sorry @bruAristimunha, missed your comment. The goal is to automatically populate all the signal-related parameters when we can and not break things when not possible.

[sliwy]

I would restore a test with initialized nn.Module provided to the EEGClassifer - the one that was failing before (for example test_post_epoch_train_scoring).
Let's check that the warning is shown and that the model is not reinitialized with default params.

Apart from that and some details (flake8, docstring and test with only pass) everything looks good to me! :)

[robintibor]

Hey @PierreGtch and @sliwy,
Please ping me if you need any help to solve the design conflict. To be honest, I looked the discussion at the PierreGtch#3, and I still don't get the big picture. I am available to chat or meet if necessary.

Sorry @bruAristimunha, missed your comment. The goal is to automatically populate all the signal-related parameters when we can and not break things when not possible.

I missed this, but would prefer we don't do that (deprecating passing initialized module). I think there is a tradeoff between automating more stuff (like setting parameter according to dataset) and being simpler/more transparent. So in my view should be fine to also pass initialized module, and in that case as is now, do not set parameters according to dataset... no reason to break this workflow I think...

[PierreGtch]

@robintibor I can just turn the warning into an info and remove the deprecated part

[robintibor]

sounds sensible

[PierreGtch]

Then I'm ready to merge

[bruAristimunha]

Hi @sliwy!

I enabled auto merge, if good for you, just approve and will be merged.

[robintibor]

Would be good to have some docs example for usage I think? To see how it all works?

[PierreGtch]

Yes it's planned but I will wait for #529 and #528

[bruAristimunha]

What's new checker is working (I hope) @PierreGtch