@TonyBagnall, any good ideas about the error?

@TonyBagnall, any good ideas about the error?

it seems to be failing on an azure test, I cant even find which one it is failing, all I can see when I click on the above is "TypeError: object of type 'float' has no len()" in test_stat. I have no working knowledge of azure, sorry. I'll take a look if you like

The really odd thing about the failing test is that it's coming from the orchestration/benchmarking module, while all time series classifier related tests pass as it would be expected.

@ViktorKaz, do you have an idea what is going on here?
@TonyBagnall, @mloning, any comments?

it seems to be failing on an azure test, I cant even find which one it is failing

The name of the test is test_stat in test_orchestration.py - I believe it's failing locally too, not just on Azure, @TonyBagnall?

@fkiraly I cloned your PR and ran the failing test locally. I get the following error:

ModuleNotFoundError: No module named 'sktime.distances.elastic_cython'

@fkiraly I cloned your PR and ran the failing test locally. I get the following error:

The likely cause of this is your C compilers not being up to date? If you are using windows, make sure you have VS build tools installed.

Check the tests if you want to see which one failes - e.g., to on build-and-test and "details"

@fkiraly I am using Linux and generally don't have issues with Cython or the C compilers with other PR. I tried to clone your PR in order to be able to debug it and got the error in question.

Just by interpreting the tests output and code in GitHub, len is used in sktime.benchmarking.orchestration only to check whether the length of the lists with the strategies, tasks and datasets is the same. It is possible that one of these variables was defined as a float rather than a list. However, it is hard to say where the error is coming from without cloning the PR and running the test locally.

I can try to have a closer look if you want.

I can try to have a closer look if you want.

That's really strange - I assume you can access the test logs and see that this doesn't get the same error you describe?
I can't explain it.

Thanks for offering to have a look, that would be great - TSC refactor would unlock a number of interesting items to work on.

the problem I envisage with handling conversion in the base class comes when we adapt to handle unequal length series. Suppose the classifier always wants to pull the data from pandas into a numpy array when it is given equal length problems, due to the gross inefficiency of not doing so. However, if the data is not equal length, it cannot do this and must dictate how to convert the data into a compatible data structure. This data structure could be completely different for different classifiers, and may differ if, for example, dimensions are different lengths in addition to series. This would not be a problem if we separate out the checking (done in fit) from the conversion (done in _fit and possibly bespoke).

This would not be a problem if we separate out the checking (done in fit) from the conversion (done in _fit and possibly bespoke).

Well, currently there is no conversion in this PR. This aims to be a 1:1 replication of current behaviour with the main change being purely internal: namely, generic input logic that's already there moving to the base class.

My opinion on the "next bit", i.e., conversion and different input handling:

• have default conversion machinery in fit, similar to Forecasting support for multivariate y and multiple input/output types - working prototype #980
• allow "bypass" of the conversion machinery by having multiple "internally supported types", i.e., if the type of input is on a passlist, the input goes unchanged to _fit. Which types are internally supported is up to the concrete estimator, and encoded in a tag/capability.
• _fit aims to implement functionality for "core input types" like numpy etc, similar to @mloning's suggestion in Multivariate forecasting (prototype) #1074. Bespoke conversions can happen in there for the types that are fed through the "bypass".

Well, currently there is no conversion in this PR. This aims to be a 1:1 replication of current behaviour with the main change being purely internal: namely, generic input logic that's already there moving to the base class.

check_X(X, coerce_to_numpy=coerce_to_numpy)

can convert X to numpy if coerce_to_numpy is true. Now the issue of whether to do that is not just dependent on the type of X (convert if pandas) but also the content of X (if it contains unequal length series, may do something bespoke). Having it here means that a classifier with this condition would have to override fit(), thus making it all more complex. As I said, if the check and coerce were separated, it may not be an issue. This is future proofing against planned changes, not questioning whether it would work now.

what would be your proposed change?

Moving the coerce_to_numpy arg to a tag?

I would like to move all of the data conversion out of check_X and check_X_y. These should just check whether the classifier has the capability to deal with the input data. I would delegate all conversions to the classifiers in _fit, which could call standard converters or do bespoke operations. This seems to me to be an argument against #980 model for classification, if the variation is internal to the data structure (equal/unequal length) I dont think 980 works?

or define a bespoke time series data structure ..... https://github.com/uea-machine-learning/tsml/tree/master/src/main/java/tsml/data_containers

or define a bespoke time series data structure

No, I am against that - users expect numpy or pandas or sth very similar.

I would delegate all conversions to the classifiers in _fit, which could call standard converters or do bespoke operations

I am against the first part, that would result in a lot of boilerplate code, which is precisely what the fit/_fit refactor tried to reduce.

I agree with the second part, have standard converters and a simple interface to access these.

To align: why not have some "default conversion" in fit which avoids boilerplate but you can turn off, and you can use the standard converters in _fit too if you like?

I would like to move all of the data conversion out of check_X and check_X_y.

Agreed, it is an unexpected location, and a textbook violation of the single responsibility principle.

I would delegate all conversions to the classifiers in _fit, which could call standard converters or do bespoke operations

I am against the first part, that would result in a lot of boilerplate code, which is precisely what the fit/_fit refactor tried to reduce.

I think its inevitable that classifiers will handle the same data structures (so called panel data, aka 3D arrays) in different ways dependent on the contents. Without a bespoke data structure it is inevitable you have to delegate to the classifiers. If the data is in a panda and is equal length, most classifiers will pull it out into a 3D numpy. If it is unequal length, it is classifier dependent. It may leave it in the panda, extract it to a list of arrays, pad it with a transformer into equal length or indeed use some ragged array solution such as awkward arrays. The same classifier could have an option to do any one of these things, dependent on the algorithm selected. None of this is defined, but will be considered soon. I think until these use cases are sorted out, we should not perform any data structure conversions in the base class.

@TonyBagnall, I think the thought of moving to a silver bullet format/standard to solve problems with multiple appropriate but divergent formats/standards is very seductive but notoriously misleading...

The same classifier could have an option to do any one of these things, dependent on the algorithm selected. None of this is defined, but will be considered soon. I think until these use cases are sorted out, we should not perform any data structure conversions in the base class.

What do you mean with use cases here?

@TonyBagnall, more thoughts about the data structure, and I agree more with you than I did above.
I think we get something very nice if we combine your idea with @ninamiolane's geomstats architecture (which is similar to @ablaom's for data tables).

With the fit/_fit architecture, it would be easy to wrap data objects inside a "scitype" class with a unified interface. This architecture could be similar to that in @ninamiolane's interface geomstats package, and may also allow work with lazy data loading or tensor processing back-ends. We could even 1:1 reuse @ninamiolane's back-end in many parts.

Unlike in geomstats, though, the user wouldn't even necessarily see it, because fit could wrap it if the object passed is not already wrapped.

I now even think this may be the "end state" architecture we should be aiming for.

What do you think of this architecture, @ninamiolane?
(let me know if you need more context, happy to write up an extension proposal which explains it in detail)

With #980, we wouldn't even need to implement it for all estimators at the same time, but could introduce support for the data wrapper step-by-step. This is because all types would be supported at all times as long as all conversions have been implemented at the start, so no change in the user-sided interface would occur between updates and releases.

So, as a sketch, in the vanilla version of the idea, the fit would look like:

def fit(self, X, y):

    X = some_check(X)
    y = some_check(y)

    X = NinaDataWrapper(X)
    y = NinaDataWrapper(y)

    self._fit(X, y)

Combined with #980, we would have:

def fit(self, X, y):

    X = some_check(X)
    y = some_check(y)

    X = convert(X, self.get_tag("inner_type_X"))
    y = convert(y, self.get_tag("inner_type_y"))

    self._fit(X, y)

where for the classifiers that have been "upgraded", the "inner_type" would be NinaDataWrapper, and convert would do the conversion. For the classifiers that have not been upgraded, convert outputs whichever type the inner _fit can deal with, e.g., 3D numpy array or data frame. Since we can switch the inner_type over one-by-one, the user wouldn't notice anything (except an increas in efficiency).

Do you see the same on your side?

@ViktorKaz, no - this looks like a problem with your fbprophet installation? You may try the fixes in the installation guidelines; an updated version (not merged yet) is in #1103, this may even be more helpful to you, it lists 3 workarounds for the prophet issue.

@TonyBagnall, it looks like the problem is coming from ProximityForest, not from the orchestration suite.
Switching out ProximityForest for KNearestNeighbor seems to solve it, so it's definitely something with the proximity forest.

I haven't tracked it down to debug it, but the earlier commit should give you code to reproduce it.

I'm a bit confused, there are long engineering conversations with some bug talk embedded in it
"@TonyBagnall, it looks like the problem is coming from ProximityForest, not from the orchestration suite.
Switching out ProximityForest for KNearestNeighbor seems to solve it, so it's definitely something with the proximity forest.

I haven't tracked it down to debug it, but the earlier commit should give you code to reproduce it."

could you raise an issue describing the bug please?

could you raise an issue describing the bug please?

as said, I have not isolated the issue, but it's in the interaction of the orchestration framework and the ProximityForest.

There is a ProximityForest used in the current orchestration test, which fails after the refactor. If it is switched out for basically any other time series classifier, the orchestration test passes. A medium-depth look at the tracebeek seems to suggest that it is some unexpected behaviour of ProximityForest rather than of the orchestration framework.

Happy to raise a bug report.

Do you see the same on your side?

@ViktorKaz, I noticed that my earlier comment was incorrect.

The 44 tests failing were probably due to one of the tags not being correctly registered in the new registry. The registry was created after this PR, so merging from main caused those tests to fail, while before there was just one.

To reproduce the bug that was earlier here, look at my description to reproduce in #1114.

Sorry for the confusion.

Thank, I have not been involved with this code at all, so will try find someone to look into it

@fkiraly no problem, I see that you sorted out the failing test.

@fkiraly no problem, I see that you sorted out the failing test.

Well, I sorted it by switching the estimator causing the problem to one of the others, but it didn't really "solve" the problem.
What it did, I think, it removed the problem from causing a secondary problem in the test.

@TonyBagnall, would you mind re-reviewing this? The test bug is a separate thing, I'll add details in #1114.
The aim of this is to prepare a refactor similar to #955.

@TonyBagnall, would you mind re-reviewing this? The test bug is a separate thing, I'll add details in #1114.
The aim of this is to prepare a refactor similar to #955.

np, I'll look this afternoon