For the first task

Moving the convert_y_to_keras function in BaseDeepClassifier into a utils function

Any idea exactly where should I place this utils function?

For GPU functionality:

1. A user would need tensorflow-gpu and cuda-toolkit installed. We will have to add them as a soft dependency? I am unsure if these are required since pytorch (which I am comfortable with) does not really require them
2. We can also have the functionality of multi-gpu: https://keras.io/guides/distributed_training/ . In my estimate majority of the people using sktime would be on kaggle / collab in which case there is a single gpu used. And time series data should not be so heavy that multi-gpu are required, but there might be few using it on larger data / maybe college gpu cluster where they might have easy access to gpus and would prefer running the models with multiple gpus? It might be worth having in those cases

Update: I am also ignoring tests test_fit_does_not_overwrite_hyper_params for CNNClassifier since that is giving issues wrt pickling tf models (more specifically, it is unable to pickle a lambda funciton which is used for optimizer).
Stack overflow mentions that pickle by default does not support lambda functions, and doing any work around for the same would require going too deep into tf implementation instead.

Update: I am also ignoring tests test_fit_does_not_overwrite_hyper_params for CNNClassifier

What is the problem with that test?

The test uses pickle to save the model, and I think once we update the save function as per #3022 we can stop ignoring it.
The error it gives:

    raise PicklingError(
_pickle.PicklingError: ("Can't pickle <function make_gradient_clipnorm_fn.<locals>.<lambda> at 0x7f780541a3a0>: it's not found as keras.optimizers.optimizer_v2.utils.make_gradient_clipnorm_fn.<locals>.<lambda>", 'PicklingError while hashing <keras.optimizers.optimizer_v2.adam.Adam object at 0x7f78041cefd0>: PicklingError("Can\'t pickle <function make_gradient_clipnorm_fn.<locals>.<lambda> at 0x7f780541a3a0>: it\'s not found as keras.optimizers.optimizer_v2.utils.make_gradient_clipnorm_fn.<locals>.<lambda>")')

Hey @fkiraly @TonyBagnall ,
Any idea why the single test is failing for mac? I can not find any leads

re single test, for anyone who reads this, it was discussed on slack, and is related to the sporadic failure #2368

Also, question: can you explain why the test test_fit_does_not_overwrite_hyper_params did not fail prior to this PR? Which change makes it fail? Perhaps that could be avoided somehow?

From what I know, it is because of specifying lr in the optimizer by default. The full error description is this

'test_fit_does_not_overwrite_hyper_params[CNNClassifier-ClassifierFitPredictMultivariate]': PicklingError("Can't pickle <function make_gradient_clipnorm_fn.<locals>.<lambda> at 0x7f0a6918aaf0>: it's not found as keras.optimizers.optimizer_v2.utils.make_gradient_clipnorm_fn.<locals>.<lambda>", 'PicklingError while hashing <keras.optimizers.optimizer_v2.adam.Adam object at 0x7f0a6917e940>: PicklingError("Can\'t pickle <function make_gradient_clipnorm_fn.<locals>.<lambda> at 0x7f0a6918aaf0>: it\'s not found as keras.optimizers.optimizer_v2.utils.make_gradient_clipnorm_fn.<locals>.<lambda>")')

On a bit of investigation, the error is when I use Adam (or any optimizer) and store it as a class variable. For example, in the current code I am doing self.optimizer = Adam() in line 140, which leads to the test_fit_does_not_overwrite_hyper_params failure. On the other hand, if I were to not write that line, and instead directly assign Adam within the model.compile at line 149, it will show all tests passed.

Directly specifying the optimizer is not feasible, since we want to give the option to specify the optimizer to the user. Ofcourse, if instead of optimizer we were to directly take input of learning_rate we can do it, but it feels like a work around, and it would take away the choice of optimizer from the user (although Adam is the most popular one)

Ah, I see.

An sklearn interface requirement is to never overwrite self.paramname if paramname is an __init__ arg.

The test is failing correctly and pointing out that you are not interface compliant!

Instead, write the optimizer to self.optimizer_ in __init__, and overwrite that if it is None, so self.optimizer remains unchanged.

PS: generally, please do not except tests if are not sure what the tests are testing. Please remove the test skip again.

@fkiraly
Updated the code, it works now. Thanks for the suggestion!

Hey, before merging this I am planning to make a couple more design changes. I will formalise it in a bit, just informing so we dont merge this

@fkiraly made some changes:
I converted label_encoder and onehot_encoder to class variables instead of function variables as they were. I dont know why they were function variables in the first place (maybe I did not realise it at first) but convert_y_to_keras (the function which takes in the label_encoder and onehot_encoder as input in BaseDeepClassifier) is called from CNNClassifier which itself has no parameter for label_encoder and onehot_encoder (hence, no way for the user to specify it!)

Now, I am asking for them as input in __init__ of CNNClassifier so users can specify required encoders. One thing I want you to note/comment on is line 174-175 where I am doing:

self.label_encoder = self.label_encoder_
self.onehot_encoder = self.onehot_encoder_

In cnn.py. The reason for this is so that BaseDeepClassifier can directly change self.label_encoder from the convert_y_to_keras method, and hence I do not have to send it parameters, or receive the two encoders along with y (which as you stated, is non obvious). If you think this is unnecessary, I can change it back so that we send and receive parameters, and update the documentation to as it was in the previous commit.

• can you explain what purpose exposing the label and onehot encoder in the __init__ serves? What would be a second example that makes sense here, i.e., another value that the user can pass?

My essential idea was to allow for custom functions instead of the default one by sklearn. For example, if I want to use a specific Label Encoder (and I send the encoded data to CNNClassifier), the CNNClassifier will run the sklearn's default LabelEncoder on top of it which might cause it to lose some information. Maybe a better solution would have been to have a variable OR a check telling if it has already been encoded?

Maybe a better solution would have been to have a variable OR a check telling if it has already been encoded?

Can you give two or three examples of intended usage here? Not sure whether I fully understand.

Okay so i thought a bit about it and I was wrong, I do not think it is required:
My original though revolved around this particular scenario:
Lets say my inputs are ['>90', '80-90'....'0-10'] (these are the classes I want to classify in. For some reason I want to do different types of grouping, where '50-100' becomes when group and '0-50' becomes another. In this case I would either have to change the data, or simply create a new Label Encoder which would do encodings are required. This is a customisation I thought would be useful
But, if the user has to create a Label Encoding function themself, they can easily just run the test data on it and pass it on to the CNN Classifier. Another doubt I had about information loss would not hold either, since running sklearn Label Encoder on encoded data would not give different results in such a case.
I will remove them and set the default as sklearn's LabelEncoder and OneHotEncoder

ok, also fine for me.

I don't have an opinion either way, only applying the general principle that function or init args should be justified by at least two, optimally three different, meaningful examples of values/references that users may want to pass to them.

Let me know when you think this is ready for another review.

@fkiraly it is, have made the requested changes.