Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

factorize common tests #406

Closed
mblondel opened this issue Oct 20, 2011 · 19 comments · Fixed by #893
Closed

factorize common tests #406

mblondel opened this issue Oct 20, 2011 · 19 comments · Fixed by #893
Labels
Easy Well-defined and straightforward way to resolve Enhancement

Comments

@mblondel
Copy link
Member

Easy:

  • in test_common, check that the ValueError raise has a useful error message. (see sparse test for an example)
  • put as many of the "specific" tests in test_clustering, test_transformers, ... into test_non_meta_estimators.

Not so easy:

  • calling fit forgets the previous model if any
  • check how classifiers handle only one class being present
  • test how models handle non-float input (does uint8 cause overflows?)

Things done

We should factorize common tests in a new file test_common.py (or maybe test_input.py?). Things to check:

  • can pickle the object
  • raise an exception when data contains nans
  • raise an exception for invalid input (e.g., np.matrix or sp.csr_matrix if dense only implementation)
  • raise an exception if n_features is not the same in fit and predict or transform
  • __repr__ and clone work
  • check that we can pickle and unpickle estimators.
  • check that all classifiers have a classes_ attribute (needs some fixes)

Edit by @amueller!

Edit by @GaelVaroquaux on Aug 13th 2014 to reflect progress in the codebase.

@GaelVaroquaux
Copy link
Member

Thinks to check:

That the repr actually works, and that we can 'clone' the object. In
other words, that the init does set the parameters it pretends it
does.

@amueller
Copy link
Member

Ok so #893 addressed some of these. Still todo

  • testing clustering algorithms
  • testing transformers (which are quite a lot, i.e. decomposition, manifold, preprocessing)
  • using training / test set split
  • fit forgets model
  • input validation testing
  • sparse matrix support testing

...

@amueller
Copy link
Member

From #943 (which is a duplicate of this one I guess):

  • test how classifiers handle only one class being present

- check clustering and transformer objects

  • check how non-numeric-contiguous labels are handled in classifiers

- test for consistent output shapes in regressors / classifiers / transformers

  • test how methods handle non-float input (does uint8 give overflow errors?)
  • test that all classifiers have classes_
  • test that value errors that are raised are actually informative - @GaelVaroquaux did a good job on the sparse check.

Sparse matrix support testing is done thanks to @GaelVaroquaux. Input validation testing should be good now, too.
For handling non-numeric labels and handling only one class in classification, I didn't have the impression that there is a consensus on what to do, so we have to decide that first.

@amueller
Copy link
Member

Oh and I don't really know how to see that fit forgets the model. There is no way to see which attributes are set by fit, right?
We could test the __dict__ of the model, if we wanted...

@raghavrv
Copy link
Member

@amueller

We could test the __dict__ of the model, if we wanted...

Could we test that by fit ( with x1 ) --> fit ( with x2 ) --> fit ( with x1 ) and making sure 1 and 3 are equal and 1, 2 are not?

@amueller
Copy link
Member

Yeah, that would be possible test, not super strong, though.
The other would be .fit(X1, y1).fit(X2, y2) == lfit(X3, y3).fit(X2, y2)

@raghavrv
Copy link
Member

thanks for the response!

yeah that seems better... will open a PR...

@amueller
Copy link
Member

amueller commented Apr 1, 2015

Just to bump this up again: Replacing "assert_raises" by "assert_raise_message" or "assert_raises_regexp" in the common tests should be pretty simple if any of the GSoC people are bored ;)

@vinayak-mehta
Copy link
Contributor

@amueller, you mean replacing assert_raises throughout the sklearn tests?

@raghavrv
Copy link
Member

raghavrv commented Apr 4, 2015

Yes he means replacing assert_raises with assert_raises_regexp like you did here.

@raghavrv
Copy link
Member

raghavrv commented Apr 4, 2015

Also sometimes it would be simpler to not use the full error message and use only parts of it like done here.

@vinayak-mehta
Copy link
Contributor

Ok, but what I meant was, are we in a way deprecating assert_raises from sklearn by replacing all of its instances?

@raghavrv
Copy link
Member

raghavrv commented Apr 4, 2015

Nope... We just want to assert that the code not only raises a given exception, but also raises the intended error message... This helps in making sure that the test is correct.

Say for example

KMeans(n_init=0).fit(np.random.random_sample((1, 2)))
KMeans(n_init=5).fit(np.random.random_sample((1, 2)))
KMeans(n_init=0).fit(np.random.random_sample((10, 2)))

All raise ValueError

but only the 3rd line along with assert_raises_regex correctly checks if n_init=0 raises that ValueError like tested here.

@raghavrv
Copy link
Member

raghavrv commented Apr 4, 2015

Its also probably a good idea to use assert_raises_regex instead of assert_raises_regexp. Refer this.

@amueller
Copy link
Member

amueller commented Apr 5, 2015

I was mostly talking about this file:
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/tests/test_common.py

But in general it is a good idea to be explicit about the error messages everywhere.

@vinayak-mehta
Copy link
Contributor

Couldn't find any instance of assert_raises in that file, so was a bit confused. I'll try to replace as many of the other instances as I can.

@amueller
Copy link
Member

amueller commented Apr 5, 2015

Because all the checks are in https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/estimator_checks.py sorry, I should have made that more clear.

@vinayak-mehta
Copy link
Contributor

Oops, I should've looked into that long import on Line 31. Replacing.

@amueller
Copy link
Member

Fixed in #4550

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Well-defined and straightforward way to resolve Enhancement
Projects
None yet
5 participants