[ENH][BUG] Second test parameter set for shapeDTW

[Xinyu-Wu-0000]

Towards #3429

Adds a second test parameter set for shapeDTW

[benHeid]

Hi @Xinyu-Wu-0000, thank you for your pull request. In the CI pipeline, a linting step is failing. Thus, please execute black and update this pull request :)

[fkiraly]

Guide how to set up linting locally on your computer: https://www.sktime.net/en/stable/developer_guide/coding_standards.html

[Xinyu-Wu-0000]

Thank you for your comments, I updated my linting setup and the pull request.

[fkiraly]

Nice, thanks! Started the tests.

[Xinyu-Wu-0000]

FAILED sktime/classification/tests/test_all_classifiers.py::TestAllClassifiers::test_multioutput[ShapeDTW-1] - TypeError: __init__() got an unexpected keyword argument 'n_neighbours'

Maybe the error above is not cased by my commits. Mismatch n_neighbours != n_neighbors is already in main branch:

line 41 in sktime/classification/distance_based/_shape_dtw.py: n_neighbours

line 126 in sktime/classification/distance_based/_shape_dtw.py: n_neighbors

line 241 in sktime/classification/distance_based/_shape_dtw.py: n_neighbours

[Xinyu-Wu-0000]

I have changed n_neighbours to n_neighbors since n_neighbors is used in many other classes.

[fkiraly]

Hm, looks like you found a bug! That's why it's useful to have more test parameters!
(I added two badges, for bug report and bugfix, bug and code)

Btw, can you explain why it did not fail previously? Why is the constructor with the wrong variable name never called in the first test case!

[fkiraly]

(I'll review when test results are here)

[Xinyu-Wu-0000]

Btw, can you explain why it did not fail previously? Why is the constructor with the wrong variable name never called in the first test case!

To my understanding, the constructor will only be called when shape_descriptor_function == "compound" and "metric_params["weighting_factor"] unset, then ShapeDTW will try to call the contstructor to tune the weighting_factor.

[Xinyu-Wu-0000]

For the new error UnboundLocalError: local variable '_reset' referenced before assignment in sktime/classification/distance_based/_shape_dtw.py, line 187, in _fit:

If metric_params is not None, then _shape_dtw.py, line 167, in _fit will not be excecuted, _reset will be unbound:

However, _shape_dtw.py, line 187, in _fit will always be excecuted whether metric_params is None or not.

[Xinyu-Wu-0000]

        else:
            _reset = False

is added to _fit to fix the unbound error.

Now check_estimator will return All tests PASSED!:

from sktime.classification.distance_based import ShapeDTW
from sktime.utils.estimator_checks import check_estimator

check_estimator(ShapeDTW, raise_exceptions=True)

[Xinyu-Wu-0000]

For the other error in test-mlflow, it seems the filepath extension is invalid.

"sktime/classification/deep_learning/base.py line 314" in _serialize_using_dump_func

If self.model_ is returned by self.build_model in sktime/classification/deep_learning/cnn.py line 185:

Then self.model_.save is keras.models.Model.save as defined in sktime/classification/deep_learning/cnn.py line 157

It will be keras.models.Model.save(path / "keras/").

In the test-mlflow path is PosixPath

I don't know how to fix it. What path should be? Or is the error inevitable with path / "keras/" be passed to save?

[Xinyu-Wu-0000]

The invalid filepath extension error happens in other pull requests too: test-mlflow in #6107 and test-mlflow in #6102.

[yarnabrina]

You can ignore those errors in mlflow and pandas v1 tests. Those are being tracked in #6094 and #6098 by @benHeid should fix those when merged in main.

[Xinyu-Wu-0000]

You can ignore those errors in mlflow and pandas v1 tests. Those are being tracked in #6094 and #6098 by @benHeid should fix those when merged in main.

Thank you for the guiding. All tests passed except mlflow and pandas v1. If my fix on _reset in commit 2a776ae is ok, I think this PR could be merged.

[Xinyu-Wu-0000]

Not sure about the error in job commit-and-push. It seems that the Github action tried to find the branch test_params_shape_dtw in sktime repository. Maybe it' caused by the last commit merging my fork into this PR.
One of the Limitations & Gotchas of this Github action: stefanzweifel/git-auto-commit-action@v5 is no support to integrate remote upstream changes to a local repository. From the perspective of the Github action, sktime is the local repository and my fork is the remote.

No support for git rebase or git merge. There are many strategies on how to integrate remote upstream changes to a local repository. git-auto-commit does not want to be responsible for doing that.

I am merging main into test_params_shape_dtw so that the last commit won't be merging remote into local.

[fkiraly]

@Xinyu-Wu-0000, thanks for the explanation! Help would indeed be appreciated with the workflow, I've opened a new issue here describing the problem: #6181