Repository: aeon-toolkit/aeon
Pull request #741 (2023-09-12 10:33:31)

[BUG] remove base classifier shortcut and new test
#### Reference Issues/PRs

#726 

#### What does this implement/fix? Explain your changes.

1.  Adds a guard to `_clean()` in cBOSS in case the transformer is not yet initialized.
2.  Removes the shortcut for single class cases in BaseClassifier.  After discussing with Matthew it seems like this may introduce the need for further updates on many classifiers. 

#### What should a reviewer concentrate their feedback on?

Whether or not to make the change in BaseClassifier or instead fix up cBOSS and IndividualBOSS (see comment below)

#### Did you add any tests for the change?

Yes I added a new test `test_boss_empty_fit()` in `test_boss.py`. edit: I can make this test more specific to the issue by forcing a single class in the data.

#### Any other comments?

Guarding against an empty transformer only partially resolves the bug reported in #726 (and reproduced in a test case I added).  After that, there's a new error when the ContractableBOSS code tries to calculate accuracy of the IndividualBOSS instances.  This is due to the same root cause though:

The root cause is that because IndividualBOSS subsamples, there are situations where it tries to fit data with a single class.  This causes the BaseClassifier to not call `_fit()` on the IndividualBOSS instance.  The `_transformer` and `_transformer_data` properties of IndividualBOSS are initialized to `None` and `[]` respectively in the `__init__()` and then updated in `_fit()`.  Because `_fit()` is never called though, the IndividualBOSS instance doesn't update those two variables and this causes breaks further down.

In this PR I'm proposing to changing BaseClassifier mainly to initiate a discussion on the topic but I think an equally viable path is to go into IndividualBOSS and make sure that `_transformer_data` and `_transformer` are initialized in the `__init__` in such a way that the rest of the code works.  I've already started looking into that and don't think it's a huge lift (probably a lot simpler than the refactor implied by updated BaseClassifier tbh).  Happy to update this PR with that instead of this change to `BaseClassifier`.

Happy to discuss on slack as well

### PR checklist

<!--
Please go through the checklist below. Please feel free to remove points if they are not applicable.
-->

##### For all contributions
- [ ] I've added myself to the [list of contributors](https://github.com/aeon-toolkit/aeon/blob/main/.all-contributorsrc).
- [ ] Optionally, I've updated aeon's [CODEOWNERS](https://github.com/aeon-toolkit/aeon/blob/main/CODEOWNERS) to receive notifications about future changes to these files.
- [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.
- [ ] For new estimators, I've added the estimator to the online documentation.


<!--
Thanks for contributing!
-->


MatthewMiddlehurst: If anyone has any comments on the current method for dealing with single class label input i.e. @TonyBagnall or @patrickzib feel welcome to post here.
TonyBagnall: > If anyone has any comments on the current method for dealing with single class label input i.e. @TonyBagnall or @patrickzib feel welcome to post here.

I think we should keep it for now, since it deals with things that can break in fairly obscure places. 
hedeershowk: Sorry for the late response here.  So how about I revert the base changes and as Matthew suggested move the test and keep the specific fix for the bug?
MatthewMiddlehurst: Yes I think keep the base as is for now.
TonyBagnall: > Sorry for the late response here. So how about I revert the base changes and as Matthew suggested move the test and keep the specific fix for the bug?

sounds good to me, thanks. 
TonyBagnall: > Sorry for the late response here. So how about I revert the base changes and as Matthew suggested move the test and keep the specific fix for the bug?

hi @hedeershowk are you still working on this? Can we help in any way?
hedeershowk: > > Sorry for the late response here. So how about I revert the base changes and as Matthew suggested move the test and keep the specific fix for the bug?
> 
> hi @hedeershowk are you still working on this? Can we help in any way?

Sorry I haven't followed up.  I can clean it up and PR again in the next few days but if it's holding anything back please obviously go ahead.  Apologies for such a long delay here.
MatthewMiddlehurst: Thanks for the reply, no rush really. Just checking in to see if you still want to work on it 🙂 

MatthewMiddlehurst: I have resolved the conversations from before, I think we want to keep the base class functionality for now. This can cause some issues, but with proper testing they should be caught.
MatthewMiddlehurst: @all-contributors please add @hedeershowk for code.
allcontributors[bot]: @MatthewMiddlehurst 

I've put up [a pull request](https://github.com/aeon-toolkit/aeon/pull/1078) to add @hedeershowk! :tada:

Patch:
diff --git a/aeon/classification/dictionary_based/_boss.py b/aeon/classification/dictionary_based/_boss.py
index b164fbc3..0f2e1806 100644
--- a/aeon/classification/dictionary_based/_boss.py
+++ b/aeon/classification/dictionary_based/_boss.py
@@ -687,6 +687,8 @@ class IndividualBOSS(BaseClassifier):
         return new_boss
 
     def _clean(self):
+        if self._transformer is None:
+            return
         self._transformer.words = None
         self._transformer.save_words = False
 
diff --git a/aeon/classification/dictionary_based/_cboss.py b/aeon/classification/dictionary_based/_cboss.py
index 201b4b41..a174fb7f 100644
--- a/aeon/classification/dictionary_based/_cboss.py
+++ b/aeon/classification/dictionary_based/_cboss.py
@@ -336,6 +336,9 @@ class ContractableBOSS(BaseClassifier):
         """
         sums = np.zeros((X.shape[0], self.n_classes_))
 
+        if self.n_estimators_ == 0:
+            return sums + 1 / self.n_classes_
+
         for n, clf in enumerate(self.estimators_):
             preds = clf.predict(X)
             for i in range(X.shape[0]):
@@ -409,8 +412,12 @@ class ContractableBOSS(BaseClassifier):
         correct = 0
         required_correct = int(lowest_acc * train_size)
 
-        # there may be no words if feature selection is too aggressive
-        if boss._transformed_data.shape[1] > 0:
+        # there may be no words if feature selection is too aggressive or
+        # subsampling is too small
+        if (
+            not isinstance(boss._transformed_data, list)
+            and boss._transformed_data.shape[1] > 0
+        ):
             distance_matrix = pairwise_distances(
                 boss._transformed_data, n_jobs=self.n_jobs
             )


Test patch:
diff --git a/aeon/classification/dictionary_based/tests/test_boss.py b/aeon/classification/dictionary_based/tests/test_boss.py
index a0fb5fbb..29228ee6 100644
--- a/aeon/classification/dictionary_based/tests/test_boss.py
+++ b/aeon/classification/dictionary_based/tests/test_boss.py
@@ -1,9 +1,11 @@
 """BOSS test code."""
+
 import numpy as np
 from sklearn.metrics import accuracy_score
 
-from aeon.classification.dictionary_based import BOSSEnsemble
+from aeon.classification.dictionary_based import BOSSEnsemble, ContractableBOSS
 from aeon.datasets import load_unit_test
+from aeon.testing.utils.collection import make_2d_test_data
 
 
 def test_boss_train_estimate():
@@ -22,3 +24,11 @@ def test_boss_train_estimate():
     assert train_probas.shape == (20, 2)
     train_preds = boss.classes_[np.argmax(train_probas, axis=1)]
     assert accuracy_score(y_train, train_preds) >= 0.6
+
+
+def test_cboss_small_train():
+    """Test with a small amount of train cases, subsampling can cause issues."""
+    X, y = make_2d_test_data(n_cases=3, n_timepoints=20, n_labels=2)
+    cboss = ContractableBOSS(n_parameter_samples=10, max_ensemble_size=3)
+    cboss.fit(X, y)
+    cboss.predict(X)