let me know when you would like a review!

@fkiraly, I am trying to run some of the tests that are failing locally but I am having some trouble. One of the failing tests is:

sktime/annotation/tests/test_all_annotators.py::TestAllAnnotators::test_predict_segments[GaussianHMM]

I am trying to recreate this test with the following code:

>>> from sktime.annotation.tests.test_all_annotators import TestAllAnnotators, AnnotatorsFixtureGenerator
>>> from sktime.annotation.hmm_learn.gaussian import GaussianHMM
>>> results = TestAllAnnotators().run_tests(GaussianHMM, tests_to_run="test_predict_segments")
>>> results
{'test_predict_segments[AnnotatorPipeline-0]': 'PASSED', 'test_predict_segments[AnnotatorPipeline-1]': 'PASSED', 'test_predict_segments[AnnotatorPipeline-2]': 'PASSED'}

By putting a breakpoint at the start of the test_predict_segments test, I found that the fitted_estimator argument passed to the test are instances of a SubLOF pipeline. Here is an example of one of these pipelines:

AnnotatorPipeline(steps=[('transformer',
                          TabularToSeriesAdaptor(transformer=StandardScaler())),
                         ('anomaly',
                          SubLOF(n_neighbors=5, novelty=True,
                                 window_size=datetime.timedelta(days=25)))])

I am unsure why this AnnotatorPipeline is being passed to the test considering I gave GaussianHMM to run_tests. My intuition tells me the issue is with the _generate_fitted_estimator method that I have created in this pull request but I have not had time to check this yet. If you have any ideas please let me know.

My high-level guess would be, there's something about identity of estimators that links the fitted and unfitted objects, i.e., identical objects end up in the fixtures where you would have wanted copies, and that has side effects.

This has been a persistent problem btw, I have not figured out how to pass fitted estimators to some tests, to avoid refitting - this is why none of the other test suits pass fitted estimtors as fixtures.

Based on this - unless you want to dig into the functioning of the test suite - I would recommend to always use unfitted estimator instances, or classes, and not modify the test class logic.

Thanks for the suggestions.

If you find out how we can cache fitted estimators without running into memory problems (e.g., avoiding that all estimators are cached at once, including fitted deep learning and foundation models), or "change by reference" or mutability problems, that would be a great contribution! As said, I could not find out how to do this with pytest or the custom extensions we are currently using.

Okay, I'll take a look at this and see what I can find. If I think it is too hard I'll open a separate issue and leave it for another day. I am also going to log some of findings here in this thread so I don't lose it:

The _generate_fitted_estimator function calls _generate_estimator_instance which produces a list of instantiated instances. This list does not include any of the HMM models despite using,

TestAllAnnotators().run_tests(GaussianHMM, tests_to_run="test_predict_segments")

I think the reasons these HMM models were not generated on my machine is because I didn't have hmmlearn installed and so these estimators were removed on this line https://github.com/Alex-JG3/sktime/blob/8cd1fdbfb710ad12c30ffb19b4af544a613f2fdf/sktime/tests/test_all_estimators.py#L220.

I will try installing hmmlearn and see what effect that has.

Installing hmmlearn has helped. Here is the output from running the same code from my previous comment.

>>> from sktime.annotation.tests.test_all_annotators import TestAllAnnotators, AnnotatorsFixtureGenerator
>>> from sktime.annotation.hmm_learn.gaussian import GaussianHMM
>>> results = TestAllAnnotators().run_tests(GaussianHMM, tests_to_run="test_predict_segments")
>>> results
{'test_predict_segments[AnnotatorPipeline-0]': ValueError('min() arg is an empty sequence'), 'test_predict_segments[AnnotatorPipeline-1]': 'PASSED', 'test_predict_segments[AnnotatorPipeline-2]': 'PASSED', 'test_predict_segments[GMMHMM-0]': AssertionError(), 'test_predict_segments[GMMHMM-1]': AssertionError(), 'test_predict_segments[GaussianHMM]': AssertionError(), 'test_predict_segments[PoissonHMM]': AssertionError(), 'test_predict_segments[PyODAnnotator]': TypeError("'>' not supported between instances of 'Timestamp' and 'int'")}

I am still not certain why the annotator pipelines are being included given that I have asked for only the GaussianHMM tests to run.

that is indeed strange. Have you tried on main?

If it happens on main, it is a bug in the main code base and not related to your PR. Otherwise, changes in this Pr might be causing or bringing it to the surface.

At a first glance, it doesn't seem like this is happening on main. Here is an example that I ran on main:

>>> from sktime.annotation.tests.test_all_annotators import TestAllAnnotators, AnnotatorsFix
>>> from sktime.annotation.hmm_learn.gaussian import GaussianHMM 
>>> results = TestAllAnnotators().run_tests(GaussianHMM, tests_to_run="test_output_tags")
>>> print(results)
{'test_output_type[GaussianHMM]': 'PASSED'}

I am going to work under the assumption that the issue was caused by a change made in this PR for now.

I think I have an idea what is happening. I'll explain it through two examples.

Example 1

This examples runs the test_annotator_tags test which relies on the estimator_class fixture.

>>> from sktime.annotation.tests.test_all_annotators import TestAllAnnotators, AnnotatorsFixtureGenerator
>>> from sktime.annotation.hmm_learn.gaussian import GaussianHMM
>>> results = TestAllAnnotators().run_tests(GaussianHMM, tests_to_run="test_annotator_tags")
>>> print(results)
{'test_annotator_tags[GaussianHMM]': 'PASSED'}

This works as expected. The run_tests function calls the create_conditional_fixtures_and_names function which, as the name suggests, creates the correct fixtures. This function has the parameter generator_dict which, for this example, looks like this:

{
  'estimator_class': <function QuickTester.run_tests.<locals>._generate_estimator_class at 0x7f7b56663d90>,
  'estimator_instance': <function QuickTester.run_tests.<locals>._generate_estimator_instance_cls at 0x7f7afd2be320>, 
  'fitted_estimator': <bound method AnnotatorsFixtureGenerator._generate_fitted_estimator of <sktime.annotation.tests.test_all_annotators.TestAllAnnotators object at 0x7f7afd287040>>,
  'method_nsc': <bound method BaseFixtureGenerator._generate_method_nsc of <sktime.annotation.tests.test_all_annotators.TestAllAnnotators object at 0x7f7afd287040>>,
  'method_nsc_arraylike': <bound method BaseFixtureGenerator._generate_method_nsc_arraylike of <sktime.annotation.tests.test_all_annotators.TestAllAnnotators object at 0x7f7afd287040>>,
  'scenario': <bound method BaseFixtureGenerator._generate_scenario of <sktime.annotation.tests.test_all_annotators.TestAllAnnotators object at 0x7f7afd287040>>
}

This dictionary contains the generators for generating annotator classes, annotator instances, and the fitted annotators. Notice that value for the key "estimator_class" is the _generate_estimator_class which is defined inside the run_tests function. Note the that BaseFixtureGenerator has a method with the same name here.

The TestAllAnnotators class defined in this PR inherits from both BaseFixtureGenerator and QuickTester. I think having two methods called _generate_estimator_class is causing confusion.

Example 2

This examples runs the test_predict_segments test which relies on the new fitted_estimator fixture.

>>> from sktime.annotation.tests.test_all_annotators import TestAllAnnotators, AnnotatorsFixtureGenerator
>>> from sktime.annotation.hmm_learn.gaussian import GaussianHMM
>>> results = TestAllAnnotators().run_tests(GaussianHMM, tests_to_run="test_predict_segments")
>>> print(results)
{
  'test_predict_segments[AnnotatorPipeline-0]': 'PASSED',
  'test_predict_segments[AnnotatorPipeline-1]': 'PASSED',
  'test_predict_segments[AnnotatorPipeline-2]': 'PASSED',
  'test_predict_segments[GMMHMM-0]': AssertionError(),
  'test_predict_segments[GMMHMM-1]': AssertionError(),
  'test_predict_segments[GaussianHMM]': AssertionError(),
  'test_predict_segments[PoissonHMM]': AssertionError(),
  'test_predict_segments[PyODAnnotator]': TypeError("'>' not supported between instances of 'Timestamp' and 'int'")
}

This test calls the annotator pipelines when it should only use gaussian HMM. Again, we print the contents of generator_dict which is a variable passed to create_conditional_fixtures_and_names like we did in example 1. Here is the contents of generator_dict:

{
  'estimator_class': <function QuickTester.run_tests.<locals>._generate_estimator_class at 0x7f9da71dfd90>,
  'estimator_instance': <function QuickTester.run_tests.<locals>._generate_estimator_instance_cls at 0x7f9d4de32320>,
  'fitted_estimator': <bound method AnnotatorsFixtureGenerator._generate_fitted_estimator of <sktime.annotation.tests.test_all_annotators.TestAllAnnotators object at 0x7f9d4ddfaec0>>,
  'method_nsc': <bound method BaseFixtureGenerator._generate_method_nsc of <sktime.annotation.tests.test_all_annotators.TestAllAnnotators object at 0x7f9d4ddfaec0>>,
  'method_nsc_arraylike': <bound method BaseFixtureGenerator._generate_method_nsc_arraylike of <sktime.annotation.tests.test_all_annotators.TestAllAnnotators object at 0x7f9d4ddfaec0>>,
  'scenario': <bound method BaseFixtureGenerator._generate_scenario of <sktime.annotation.tests.test_all_annotators.TestAllAnnotators object at 0x7f9d4ddfaec0>>
}

The value for estimator_class again points to the _generate_estimator_class method defined in run_tests. But, the _generate_fitted_estimator function uses _generate_estimator_class from BaseFixtureGenerator NOT from run_tests.

Short Summary

The test_annotator_tags which uses the estimator_class fixture ends up using the _generate_estimator_class defined inside the run_tests method of QuickTester. However, the _generate_fitted_estimator method created in this PR calls the _generate_estimator_class from BaseFixtureGenerator which I think is what causes the unwanted annotators to be tested in test_predict_segments.

If you look at the run_tests version of _generate_estimator_class, it only returns the estimator class for the estimator passed to run_tests. The _generate_estimator_class obviously does not use the estimator passed to run_tests.

@fkiraly, does this make sense and what are your thoughts on this?

Yes, this makes absolutely sense!

The run_tests intentionally overrides the estimator class/instance generator with one that restricts to the class in question only.

With you adding a new "fitted estimator" fixture, this mechanism is not implemented for the fitted estimator.

I would hence strongly suggest to split the current PR in two parts:

• one without the "fitted estimator" fixture, use estimator_instance and a suitable "fit" call instead. I would recomment to prioritize this
• one later about introducing the fitted estimator. We need to check if this causes memory leaks - it had caused memory leaks when I tried a while ago, I vaguely remember, so this may end up in a rabbit hole.

Looks like the remaining issues are API non-compliances by concrete estimators, right?

Yes I think so. I am going to add the failing estimators to the list of excluded estimators for the time being to try and get the pipeline to pass.

@Alex-JG3, I think this is ready now? Let's merge, so other people can add more tests.

@Alex-JG3, I think this is ready now? Let's merge, so other people can add more tests.

Yeah, I think this is ready. I can't merge since it still needs a review.