@yarnabrina @fkiraly I had to reopen it after syncing, as the last one closed automatically.

It's still work in progress, I have to implement proper naming of the columns (which was the main goal).

I also think of passing a dict of variables to _transform_case methods, and creating a wrapper for the original functions which would unpack needed variables from **kwargs passed to each _get_feature_function(feature)(*args) call. What do you think about it @fkiraly ?

Also, what about #5990?

Also, what about #5990

This also applies to Catch22 - it implements an unnecessary loop across time series instances - and could be much simplified.

I've done the same recently with Catch22Wrapper: #5983, the recipe is:

• change tags: X_inner_mtype to pd.Series or pd.DataFrame (whichever more convenient), change univariate-only to True
• remove all loops over instances and variables
• deprecate n_jobs, add the deprecation message, switch use of n_jobs to use set_config for the time of the deprecation

It's up to you whether you do this in one PR, separate PR, or not at all - it's probably easier to separate this out, if you're changing logic as well.

FYI, linting (code formatting) checks are failing, see here for a guide how to ensure you follow these, and how to set up the checks locally: https://www.sktime.net/en/stable/developer_guide/coding_standards.html

FYI, linting (code formatting) checks are failing, see here for a guide how to ensure you follow these, and how to set up the checks locally: https://www.sktime.net/en/stable/developer_guide/coding_standards.html

Yes, I changed the typing from list to List so I hope it works now.

Also, what about #5990

This also applies to Catch22 - it implements an unnecessary loop across time series instances - and could be much simplified.

I've done the same recently with Catch22Wrapper: #5983, the recipe is:

• change tags: X_inner_mtype to pd.Series or pd.DataFrame (whichever more convenient), change univariate-only to True
• remove all loops over instances and variables
• deprecate n_jobs, add the deprecation message, switch use of n_jobs to use set_config for the time of the deprecation

It's up to you whether you do this in one PR, separate PR, or not at all - it's probably easier to separate this out, if you're changing logic as well.

My only concern is that right now I use it in my project for multivariate time series, with a nested_univ dataframe as input. How would you recommend dealing it with it? Should the implementation allow the multivariate dataframe, or it would be better to run this the transform method in a loop and concat columns in the end (in my final implementation of this method in my project, not here)? It wouldn't be very elegant I'm afraid.

Output like in https://www.sktime.net/en/stable/examples/transformation/feature_extraction_with_tsfresh.html (Multivariate time series classification data) would be ideal for me. - InputColumnName_FeatureName (or its index like in your implementation here: #5983)

My only concern is that right now I use it in my project for multivariate time series, with a nested_univ dataframe as input.

Can you explain how/why this would pose a problem? You can still pass nested dataframes or other sktime compatible containers after the proposed refactor, it's just that the iteration would be handled by the base class boilerplate.

The column naming is also automatic, it is variablename__featurename if the iteration is done by the base class.

(FYI, linting again - it says it wants double quotes instead of single quotes for strings)

My only concern is that right now I use it in my project for multivariate time series, with a nested_univ dataframe as input.

Can you explain how/why this would pose a problem? You can still pass nested dataframes or other sktime compatible containers after the proposed refactor, it's just that the iteration would be handled by the base class boilerplate.

The column naming is also automatic, it is variablename__featurename if the iteration is done by the base class.

Okay great I see it now, thanks!

Excellent! Some comments.

Blocking (for discussion or change):

• check_n_jobs is no longer used, and the change seems to affect logic. I would say, let's remove it from this PR, and if you think it is needed, discuss in another PR or issue.

Okay, removed

Non-blocking:

• why do we need two dictionaries, METHODS_DICT and CATCH24_METHODS_DICT - would it not simplify multiple places? Similarly, for short feature names. Odd that these are/were treated separately.

I think it's due to the fact that std and mean weren't mentioned in the original publication: https://link.springer.com/article/10.1007/s10618-019-00647-x and were added later, so are not really a core part of catch22. That's why I created two seperate dicts, but this could be discussed in the future, but all the other packages (pycatch22, julia one etc.) also seperate the logic of core 22 features and these additional two.

• docstring should mention this also supports catch-24 features (plus mean, std)

added

• why is the catch24 argument needed and mean/std treated completely differently? That is confusing. Why not simply add mean and std as additional features to the large list? Needs to be deprecate, of course.

Same as above, maybe features should be "catch22" by default or "catch24" instead of "all", but this could be changed later in my opinion.

• outlier_norm docstring should mention which two feaures it affects.

Added

• the _transform_single_feature method is very unpleasant, as it is private, yet used in random classifiers. Not necessary for this PR, but a question: can you come up with a good way to decouple this, e.g., joint utility dispatcher or similar? It is not good style if estimtors keep calling each others' private methods, and risky. As a warning, if it is not immediately obvious, I would recommend not looking into it as things like this can develop into time consuming rabbit holes...

I spent a long time with this method which is in fact not necessary in the catch22 transformer, but I left it for compatibility with the other module. This should definetely be resolved in the future, but I didn't have time - the other module should be accordingly rewritten.

so are not really a core part of catch22. That's why I created two seperate dicts, but this could be discussed in the future, but all the other packages (pycatch22, julia one etc.) also seperate the logic of core 22 features and these additional two.

I am aware, there's catch22 and catch24. What I mean, on the code side there doesn't need to be a distinction, they are just 24 features and sometimes people only want 22 of those. Imo there's no reason to internally separate dictionaries etc, there still can be a separation in the user interface, or having only 22 as the default.

Same as above, maybe features should be "catch22" by default or "catch24" instead of "all", but this could be changed later in my opinion.

Sure - when I say "non-blocking", it means, fine if changed later or not at all.

I spent a long time with this method which is in fact not necessary in the catch22 transformer ... This should definetely be resolved in the future, but I didn't have time

Agreed - also non-blocking, as said. Just in case you had an idea on how to resolve this quickly, reading this I see that it probably is not that easy to clean this up.

I am aware, there's catch22 and catch24. What I mean, on the code side there doesn't need to be a distinction, they are just 24 features and sometimes people only want 22 of those. Imo there's no reason to internally separate dictionaries etc, there still can be a separation in the user interface, or having only 22 as the default.

Okay, do you think it could be done in the next PR in parallel with modifying the selection of features (removing catch24 bool argument)?

Okay, do you think it could be done in the next PR

Sure - when it's listed "non-blocking", it means it does not prevent from getting this PR merged if not addressed, and can well be addressed in another PR, or not a tall.

@fkiraly I have one last question, as I cannot find it in documentation -- how should the n_jobs be passed in non-deprecated way?

the warning and docstring should tell you - via set_config and backend:params. Please raise an issue if you think that information is not easily accessible or obscure, because that would be a problem (and a place for improvement).