I just had a brief glance at the former sktime-dl classifiers migrated to the sktime repository. It seems like the issue is fixed, but there is a potential footgun. I would be interested in a maintainers opinion on how to proceed. Or is my interpretation incorrect?

Potential problem

One one hand, it seems like OneHotEncoder is only used in convert_y_to_keras.

On the other hand, _predict_proba methods will presumably output a OneHot-encoded result too [1]. A naive user might be tempted to do something like this:

X_train, y_train = load_training_data(...)  # y is encoded as categorical class labels.
X_test, y_test = load_training_data(...)  # y is encoded as categorical class labels.

clf = InceptionTimeClassifier()  # Or many other deep classifiers
clf.fit(X_train, y_train)
score = clf.score(X_test, clf.convert_y_to_keras(y_test))  # Because clf.score(X_test, y_test) will fail.

Possible solutions

• Add a fit=False keyword argument to convert_y_to_keras and only set it to true in the _fit method of the subclasses. When fit=False, the transform methods OneHotEncoder and LabelEncoder are called instead of fit_transform.
• Make convert_y_to_keras private and let the user decide on a suitable solution.
• Offer a public and private version of convert_y_to_keras, perhaps with a more user-friendly name and only use encoder.transform in the public version.
• Live with the risk of users refitting the label/one-hot encoder inadvertently.

[1] https://keras.io/api/layers/core_layers/dense/, see "Output shape" section:

N-D tensor with shape: (batch_size, ..., units). For instance, for a 2D input with shape (batch_size, input_dim), the output would have shape (batch_size, units).

PS: I initially observed this problem when training InceptionTime https://github.com/cedricdonie/tsc-for-wrist-motion-pd-detection/blob/main/src/models/train_model.py and fixed it for myself via sktime/sktime-dl#132. The dataset is quite large however, so it won't be trivial to reproduce.

Welcome back, @cedricdonie!

Sorry for the long silence, we migrated sktime-dl into sktime, after having built a framework which allows to manage dependencies on estimator level.

We've pinged you here
#3351 (comment)
about the fix you made to sktime-dl.

If it is still relevant, it would be greta if you could move it to sktime proper!

InceptionTimeClassifier is now in classification.deep_learning.inceptiontime.

The tests are currently skipped in tests._config, this skip should be removed once the fix is applied.

Hi @fkiraly, after migration to sktime proper, sktime/sktime-dl#132 is definitely not a valid fix anymore. Thus, I tried to understand whether this issue still remains in sktime, but would need some help by someone who knows the code and API design better (see comment above).

Hm, interesting. I think your observation - that the output of predict_proba is not compliant with user expectations, is valid, and in fact a bug, due to interface non-conformance.

I would prefer convert_y_to_keras to be entirely private, that looks like the only way the estimator will be compliant with the general interface, among the four options you listed - is that correct? Because it is the only one which does away with the expectation towards the user to call it.

Besides that, I would combine with your suggestion to not refit the encoder in predict, that avoids issues with unequal sets of classes, and ensure that _predict_proba inverts the transformation correctly.

FYI,

• @benHeid, @yarnabrina, @Abhay-Lejith, due to connection to [ENH] umbrella issue - categorical feature support #6109, categorical feature support
• @tobiasweede who migrated the estimator
• @james-large as the author and currently specified maintainer.

I would prefer convert_y_to_keras to be entirely private, that looks like the only way the estimator will be compliant with the general interface, among the four options you listed - is that correct? Because it is the only one which does away with the expectation towards the user to call it.

@fkiraly I agree with your proposal. Let's simply make it private to prevent potentially causing bugs for users.

Besides that, I would combine with your suggestion to not refit the encoder in predict, that avoids issues with unequal sets of classes, and ensure that _predict_proba inverts the transformation correctly.

I think that this is already the case, as per my above comment:
"[...] it seems like OneHotEncoder is only used in convert_y_to_keras. In turn, convert_y_to_keras is referenced by all classifiers in the _fit but not in the _predict functions."
But please check again that this is not the case.

In fact, it could be beneficial to implement a test case that ensures that predict (or perhaps _predict) is free of side-effects by predicting random data, predicting other data, and then asserting that predicting the same random data again yield the same results. Might even by suited to something along the lines of hypothesis testing.

Excellent! Would you then like to make a PR to sktime?

Recarding your comments:

• yes, _predict should be fine, as you say
• there already is a general test case which loops over all estimators in sktime, in TestAllEstimators, that checks that predict calls are side-effect free, this is test_methods_have_no_side_effects
  • if you have reason to believe this is not working, we need to investigate further. Do you have indications that there are side effects, and the test did not catch this? You can run it via check_estimator from sktime.utils.

Excellent! Would you then like to make a PR to sktime?

Yes, I created PR #6373.

Recarding your comments:

• yes, _predict should be fine, as you say

• there already is a general test case which loops over all estimators in sktime, in TestAllEstimators, that checks that predict calls are side-effect free, this is test_methods_have_no_side_effects

  • if you have reason to believe this is not working, we need to investigate further. Do you have indications that there are side effects, and the test did not catch this? You can run it via check_estimator from sktime.utils.

Great! I have no reason to believe that this is not working. And the present issue is a bug only if the user calls convert_y_to_keras himself with a y of different classes than the y used for fit.

However, we might have an issue with classifier.fit(X_train, y_train_multilabel); classifier.score(X_test, y_test_multilabel) because I am not sure what classifier.score expects from its call to predict (or _predict). I've put my thoughts about that in the PR.