Repository: sktime/sktime
Pull request: [ENH] Changes to CNN Classifier
<!--
Thanks for contributing a pull request! Please ensure you have taken a look
at our contribution guide: https://github.com/alan-turing-institute/sktime/blob/main/CONTRIBUTING.md
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.

Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #2935 


#### What does this implement/fix? Explain your changes.
<!--
A clear and concise description of what you have implemented.
-->
- [ ] Moving the `convert_y_to_keras` function in `BaseDeepClassifier` into a utils function
- [x] Enable returning the `label_encoder` and `one hot encoder` to the user from `convert_y_to_keras` (it can be useful for the users in case they want to use it to encode other data / or to decode data later on)
- [x] Add option to have multiple linear layers in output layer / different activation function. Also add the `use_bias` parameter to output layer. (simply as a customisation option to users, multiple linear layers are useful to create deeper networks depending on dataset / choice of activation function)
- [x] option to choose optimiser (usually its Adam but just for customisation's sake we can give this as option as well). Also should allow specifying `lr` either within the optimiser or as parameter to the class itself.
- [x] Accept parameter as input in `CNNClassifier` to specify `random seed`
- [ ] Train/fit the model on GPU depending on users choice (maybe take an input parameter for this as well)
- [x] Option to view / return the loss of fit (either as a graph or simply as a list / array )

##### For all contributions
- [x] The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.

<!--
Thanks for contributing!
-->


Problem statement:
[ENH] Changing and Adding features to CNN Classifier
**Is your feature request related to a problem? Please describe.**
As discussed in the daily-stand up on 05-07-12, I wanted to propose some enhancements / changes to CNN Classifier (and hence to Deep Learning based estimators)

**Describe the solution you'd like**
- [ ] Moving the `convert_y_to_keras` function in `BaseDeepClassifier` into a utils function
- [x] Enable returning the `label_encoder` and `one hot encoder` to the user from `convert_y_to_keras` (it can be useful for the users in case they want to use it to encode other data / or to decode data later on)
- [x] Add option to have multiple linear layers in output layer / different activation function. Also add the `use_bias` parameter to output layer. (simply as a customisation option to users, multiple linear layers are useful to create deeper networks depending on dataset / choice of activation function)
- [x] option to choose optimiser (usually its Adam but just for customisation's sake we can give this as option as well). Also should allow specifying `lr` either within the optimiser or as parameter to the class itself.
- [x] Accept parameter as input in `CNNClassifier` to specify `random seed`
- [ ] Train/fit the model on GPU depending on users choice (maybe take an input parameter for this as well)
- [x] Option to view / return the loss of fit (either as a graph or simply as a list / array ) / A better idea is to add a summary function which can return model summary as well as `fit` history. 

**Additional context**
These are some additional features / changes I wanted to add to the `CNNClasifier`. Any inputs / additional comments are appreciated

Hints from issue discussion:
Yes, great additions, great suggestions!

> Also should allow specifying `lr` either within the optimiser or as parameter to the class itself.

I think this one is unnecessary, because you can pass the `lr` to the optimizer, which is an object you would then pass to the classifier class constructor. We do not need two places to pass it?

Or, do you see a good argument why it would be useful to the user to pass `lr` separately? Or am I simply misunderstanding the suggestion?
all sounds like a good idea, can we keep the defaults the same to maintain reproducibility of results? 
> can we keep the defaults the same to maintain reproducibility of results?

Changing defaults would require deprecation, so I would advise against changing defaults.
Another thing I was planning was to have `validation_split` as a parameter to make it easier for users. It is simply an additional parameter for us in the `model.fit` method, and should not be difficult. Only issue with this is are we going in accordance to other sktime estimators, do they have functionality to automate validation? In which case we should probably either add it for all (simply use sklearn's `train_test_split` or not use here as well)
> Another thing I was planning was to have `validation_split` as a parameter to make it easier for users. It is simply an additional parameter for us in the `model.fit` method, and should not be difficult. Only issue with this is are we going in accordance to other sktime estimators, do they have functionality to automate validation? In which case we should probably either add it for all (simply use sklearn's `train_test_split` or not use here as well)

I think in general this is a good idea!

The specific implementation needs to be discussed though - we should not be adding parameters to `fit`, it is really important that all estimators "look the same".

What would the parameter do? And could it maybe be added as an init option?
Would be interested to see what you have in mind, if you could write it up, say, in the form of docstrings.

Commit message: converted encoders to class variables

Patch to review:
diff --git a/sktime/classification/deep_learning/base.py b/sktime/classification/deep_learning/base.py
index 987198f9952..bbdb40116c5 100644
--- a/sktime/classification/deep_learning/base.py
+++ b/sktime/classification/deep_learning/base.py
@@ -41,11 +41,8 @@ class BaseDeepClassifier(BaseClassifier, ABC):
         "python_dependencies": "tensorflow",
     }
 
-    def __init__(self, batch_size=40, random_state=None):
+    def __init__(self):
         super(BaseDeepClassifier, self).__init__()
-
-        self.batch_size = batch_size
-        self.random_state = random_state
         self.model_ = None
 
     @abstractmethod
@@ -66,9 +63,21 @@ def build_model(self, input_shape, n_classes, **kwargs):
         """
         ...
 
+    def summary(self):
+        """
+        Summary function to return the losses/metrics for model fit.
+
+        Returns
+        -------
+        history: dict,
+            Dictionary containing model's train/validation losses and metrics
+
+        """
+        return self.history.history
+
     def _predict(self, X, **kwargs):
         probs = self._predict_proba(X, **kwargs)
-        rng = check_random_state(self.random_state)
+        rng = check_random_state(self.random_seed)
         return np.array(
             [
                 self.classes_[int(rng.choice(np.flatnonzero(prob == prob.max())))]
@@ -100,23 +109,23 @@ def _predict_proba(self, X, **kwargs):
         probs = probs / probs.sum(axis=1, keepdims=1)
         return probs
 
-    def convert_y_to_keras(self, y, label_encoder=None, onehot_encoder=None):
+    def convert_y_to_keras(self, y):
         """Convert y to required Keras format."""
-        if (label_encoder is None) and (onehot_encoder is None):
-            # make the encoders and store in self
+        if self.label_encoder is None:
             self.label_encoder = LabelEncoder()
-            self.onehot_encoder = OneHotEncoder(sparse=False, categories="auto")
-            # categories='auto' to get rid of FutureWarning
-
             y = self.label_encoder.fit_transform(y)
             self.classes_ = self.label_encoder.classes_
             self.n_classes_ = len(self.classes_)
-
             y = y.reshape(len(y), 1)
-            y = self.onehot_encoder.fit_transform(y)
         else:
-            y = label_encoder.fit_transform(y)
+            y = self.label_encoder.fit_transform(y)
             y = y.reshape(len(y), 1)
-            y = onehot_encoder.fit_transform(y)
+
+        if self.onehot_encoder is None:
+            self.onehot_encoder = OneHotEncoder(sparse=False, categories="auto")
+            # categories='auto' to get rid of FutureWarning
+            y = self.onehot_encoder.fit_transform(y)
+        else:
+            y = self.onehot_encoder.fit_transform(y)
 
         return y
diff --git a/sktime/classification/deep_learning/cnn.py b/sktime/classification/deep_learning/cnn.py
index 9756c0f046d..3aef7ca2d25 100644
--- a/sktime/classification/deep_learning/cnn.py
+++ b/sktime/classification/deep_learning/cnn.py
@@ -3,6 +3,7 @@
 
 __author__ = ["James-Large", "TonyBagnall"]
 __all__ = ["CNNClassifier"]
+
 from sklearn.utils import check_random_state
 
 from sktime.classification.deep_learning.base import BaseDeepClassifier
@@ -37,6 +38,16 @@ class CNNClassifier(BaseDeepClassifier):
         fit parameter for the keras model
     optimizer       : keras.optimizer, default=keras.optimizers.Adam(),
     metrics         : list of strings, default=["accuracy"],
+    random_seed     : integer, default=None
+        random seeding for initialisation of tf
+    activation      : string or a tf callable, default="sigmoid"
+        Activation function used in the output linear layer.
+        List of available activation functions:
+        https://keras.io/api/layers/activations/
+    use_bias        : boolean, default = True
+        whether the layer uses a bias vector.
+    optimizer       : keras.optimizers object, default = Adam(lr=0.01)
+        specify the optimizer and the learning rate to be used.
 
     Notes
     -----
@@ -46,6 +57,16 @@ class CNNClassifier(BaseDeepClassifier):
 
     Adapted from the implementation from Fawaz et. al
     https://github.com/hfawaz/dl-4-tsc/blob/master/classifiers/cnn.py
+
+    Examples
+    --------
+    >>> from sktime.classification.deep_learning.cnn import CNNClassifier
+    >>> from sktime.datasets import load_unit_test
+    >>> X_train, y_train = load_unit_test(split="train", return_X_y=True)
+    >>> X_test, y_test = load_unit_test(split="test", return_X_y=True)
+    >>> cnn = CNNClassifier()
+    >>> cnn.fit(X_train, y_train)
+    CNNClassifier(...)
     """
 
     def __init__(
@@ -59,6 +80,12 @@ def __init__(
         verbose=False,
         loss="mean_squared_error",
         metrics=None,
+        random_seed=None,
+        activation="sigmoid",
+        use_bias=True,
+        optimizer_=None,
+        label_encoder_=None,
+        onehot_encoder_=None,
     ):
         _check_dl_dependencies(severity="error")
         super(CNNClassifier, self).__init__()
@@ -71,6 +98,13 @@ def __init__(
         self.verbose = verbose
         self.loss = loss
         self.metrics = metrics
+        self.random_seed = random_seed
+        self.activation = activation
+        self.use_bias = use_bias
+        self.optimizer_ = optimizer_
+        self.label_encoder_ = label_encoder_
+        self.onehot_encoder_ = onehot_encoder_
+        self.history = None
         self._network = CNNNetwork()
 
     def build_model(self, input_shape, n_classes, **kwargs):
@@ -92,22 +126,31 @@ def build_model(self, input_shape, n_classes, **kwargs):
         -------
         output : a compiled Keras Model
         """
+        import tensorflow as tf
         from tensorflow import keras
 
+        tf.random.set_seed(self.random_seed)
+
         if self.metrics is None:
             metrics = ["accuracy"]
         else:
             metrics = self.metrics
         input_layer, output_layer = self._network.build_network(input_shape, **kwargs)
 
-        output_layer = keras.layers.Dense(units=n_classes, activation="sigmoid")(
-            output_layer
+        output_layer = keras.layers.Dense(
+            units=n_classes, activation=self.activation, use_bias=self.use_bias
+        )(output_layer)
+
+        self.optimizer = (
+            keras.optimizers.Adam(learning_rate=0.01)
+            if self.optimizer_ is None
+            else self.optimizer_
         )
 
         model = keras.models.Model(inputs=input_layer, outputs=output_layer)
         model.compile(
             loss=self.loss,
-            optimizer=keras.optimizers.Adam(),
+            optimizer=self.optimizer,
             metrics=metrics,
         )
         return model
@@ -128,11 +171,14 @@ def _fit(self, X, y):
         """
         if self.callbacks is None:
             self._callbacks = []
+
+        self.label_encoder = self.label_encoder_
+        self.onehot_encoder = self.onehot_encoder_
         y_onehot = self.convert_y_to_keras(y)
         # Transpose to conform to Keras input style.
         X = X.transpose(0, 2, 1)
 
-        check_random_state(self.random_state)
+        check_random_state(self.random_seed)
         self.input_shape = X.shape[1:]
         self.model_ = self.build_model(self.input_shape, self.n_classes_)
         if self.verbose:


Code review comments:
diff_hunk: @@ -59,6 +80,12 @@ def __init__(
         verbose=False,
         loss="mean_squared_error",
         metrics=None,
+        random_seed=None,
+        activation="sigmoid",
+        use_bias=True,
+        optimizer_=None,
path: sktime/classification/deep_learning/cnn.py
text: @user1:
the init variables should have no underscore at the end.

What I meant is that these remain as you intended, without underscore.

You then copy a coerced/substituted version to the underscore version.

@author:
I have made some changes, hope it satisfies. I am still unsure about what can be done for it, should I do a `deepcopy`?