Replace implicit imports with explicit imports

[johannfaouzi]

Fixes #134

As title says, the implicit imports are replaced with explicit imports in test_estimators.py.
It was a bit hard to find some of them from scikit-learn. Let's see if it improves code coverage.

[johannfaouzi]

Well it does not. I will try to replace them in sklearn_patches.py too.

[rtavenar]

For some reason, test_estimators.py is not listed on the coverage report, which probably explains why sklearn_patches.py is almost not covered. It is unclear to me why test_estimators is ignored

[rtavenar]

Well, it is explicitly ignored in travis. Im not sure why, though. Could you check in this direction ?

[johannfaouzi]

That may be a better explanation. I will try to remove it and see how things go.

[johannfaouzi]

With the current travis configuration file, the doctests are not run, because it is specified to run only the tests in the tests folder: pytest -v tslearn tslearn/tests/

It seems like an unintended behavior imho.

I did not see the doctest option after.

[johannfaouzi]

The clustering algorithms do not pass the tests. I am a bit tired, so I may say something stupid, but predict returns the argmin of the cross-similarity matrix:
https://github.com/rtavenar/tslearn/blob/eb37c4fbebc2e575c16a955c4bb895cf016faf29/tslearn/clustering.py#L1102

So an array of non-negative integers. So the smallest label can never be -1, and may be 1 when there is noise?
https://github.com/rtavenar/tslearn/blob/eb37c4fbebc2e575c16a955c4bb895cf016faf29/tslearn/tests/sklearn_patches.py#L117-L118

[johannfaouzi]

Never mind, it seems like the tests pass for some configurations, and lead to a more reasonable code coverage. A bit weird...

[johannfaouzi]

test_all_estimators should be refactored with pytest.mark.parametrize: it would make the output cleaner and it would avoid the runtime issues.

[rtavenar]

OK, so the failing tests are related to clustering algorithms that do not assign data to every possible cluster. Maybe adding a bit more samples to the dataset could make this more stable?

And I agree with your remark on pytest.mark.parametrize.

I can try to implement fixes, but I am not sure I can push to your master. Let me know what is best for you.

[johannfaouzi]

I will try to fix this and get back to you later on. I should have created another branch, but I often forget to do so :(

[rtavenar]

Locally, if I just set the number of time series per class (for the clustering tests only) to 15, all tests pass.

[johannfaouzi]

Locally, if I just set the number of time series per class (for the clustering tests only) to 15, all tests pass.

Did you change the number of time series in _create_small_ts_dataset()?

[johannfaouzi]

My code with pytest.mark.parametrize doesn't seem to work on Python 2.7...

One issue I faced during this PR: a lot of the functions used in sklearn_patches.py and test_estimators.py are not part of the public API of scikit-learn. I have the master branch installed and some functions were removed. I copy pasted them, but overall I don't think that it will be easy to maintain them, because these functions can be removed without any deprecation warning.

Edit: Following this post on StackOverflow, I tried to swap the decorators, let's see if it works

[rtavenar]

Locally, if I just set the number of time series per class (for the clustering tests only) to 15, all tests pass.

Did you change the number of time series in _create_small_ts_dataset()?

I did the following :

def _create_small_ts_dataset(n_ts_per_class=5):
    return random_walk_blobs(n_ts_per_blob=n_ts_per_class, n_blobs=3,
                             random_state=1, sz=10, noise_level=0.025)

[johannfaouzi]

So I removed the @ignore_warnings() decorator, which enables the code to run on Python 2. KShape test fails on Python 2 only when numba is disabled, with the predicted clusters being [0, 2] instead of [0, 1, 2]...

https://travis-ci.org/rtavenar/tslearn/jobs/578251283#L822-L836

[rtavenar]

One issue I faced during this PR: a lot of the functions used in sklearn_patches.py and test_estimators.py are not part of the public API of scikit-learn. I have the master branch installed and some functions were removed. I copy pasted them, but overall I don't think that it will be easy to maintain them, because these functions can be removed without any deprecation warning.

Argh, this is an important point indeed. Not sure how to deal with it in the future...

[rtavenar]

Seems like we have an issue here: There are failing tests but the Travis indicator for those jobs is green :/

[johannfaouzi]

My code for pyts is slightly different, I don't know if that would solve this:
https://github.com/johannfaouzi/pyts/blob/5b4310f581af774eb9d4575c0e40251ef707b5c8/.travis.yml#L32-L39

[johannfaouzi]

I know nothing about bash, but the if else loop seems weird, doesn't it ?
https://travis-ci.org/rtavenar/tslearn/jobs/578251283#L1029

Unix always exits with the status of the last command in the pipeline, so if there are several commands and the last one succeeds, it doesn't matter that previous commands failed.

[johannfaouzi]

Do you agree with the way the noise is added @rtavenar ?
https://github.com/rtavenar/tslearn/blob/eb37c4fbebc2e575c16a955c4bb895cf016faf29/tslearn/tests/sklearn_patches.py#L78

Right now it adds new points that are noise, instead of adding noise to existing points.

[rtavenar]

Do you agree with the way the noise is added @rtavenar ?

https://github.com/rtavenar/tslearn/blob/eb37c4fbebc2e575c16a955c4bb895cf016faf29/tslearn/tests/sklearn_patches.py#L78

Right now it adds new points that are noise, instead of adding noise to existing points.

Hum, this is not noise imo, this is a new dataset that includes the previous one. Not sure why we should do that instead of adding white noise to the original dataset.

[rtavenar]

My code for pyts is slightly different, I don't know if that would solve this:
https://github.com/johannfaouzi/pyts/blob/5b4310f581af774eb9d4575c0e40251ef707b5c8/.travis.yml#L32-L39

I agree that doing it this way is probably better (and is much more readable), and I guess the exit code we get is the one from the command that sends the coverage report rather than the one that corresponds to the tests themselves.

[rtavenar]

Isn't there a fi; missing here?

[rtavenar]

By the way, @johannfaouzi : all your commits are assigned to johann.faouzi (unknown github user) and hence you do not appear as a tslearn contributor, which is a pity

[johannfaouzi]

By the way, @johannfaouzi : all your commits are assigned to johann.faouzi (unknown github user) and hence you do not appear as a tslearn contributor, which is a pity

Thanks! I was wondering why, and the issue is that I was using another ID (because I once worked on GitLab and had to use my email from the institute I work in) and never changed it back to my GitHub ID...

[johannfaouzi]

I tried to ignore warnings by modifying the setup.cfg file but it didn't work. All the tests are passing (I think), but the output is ugly. I had to remove the @ignore_warnings decorator because it wasn't working well with @pytest.mark.parametrize for Python 2.7.

Let me know what you think about the current state of the PR.

[rtavenar]

Have you tried options described there ?

Like, for example, would it be a problem to disable warnings while running tests using --disable-warnings? Or use the dedicated decorator @pytest.mark.filterwarnings?

[johannfaouzi]

Thanks for the links @rtavenar! I think that it works quite well right now. I disabled:

• DeprecationWarning,
• PendingDeprecationWarning,
• sklearn.exceptions.SkipTestWarning, and
• UserWarning

There are a few RunTimeWarnings for Python 3.5 that could be worth looking at (but out of scope for this PR), and I'm not sure why it's happening for this version only.

[rtavenar]

I quite don't understand why you changed this statement, and I have to admit I also don't understand why assigned clusters could be equal to -1. Could you give a bit of insight on this?

[johannfaouzi]

I agree that a predicted label can never be -1 (since the predicted label is the argmin). I just changed what I thought to be a typo, but even with noise I think that we should expect to have at least one sample for each original cluster.

I would remove this line and replace
https://github.com/rtavenar/tslearn/blob/eb37c4fbebc2e575c16a955c4bb895cf016faf29/tslearn/tests/sklearn_patches.py#L114-L115

with

assert_array_equal(labels_sorted, np.arange(0, 3))

[rtavenar]

Makes sense to me (though maybe for some clustering methods, labels could be computed without resorting to an argmin, but I still wouldn't understand why it could be -1).

[GillesVandewiele]

Not all clustering algorithms assign each points to a cluster (and are more robust to noise). E.g. DBSCAN. The non-assigned samples get -1

[johannfaouzi]

I didn't know that! However I don't think that any of the clustering algorithm currently available in this package do that, so the changes are acceptable imho. A good thing to keep in mind for the future though :)