search

scikit-learn-contrib / lightning

Large-scale linear classification, regression and ranking in Python
https://contrib.scikit-learn.org/lightning/
1.72k stars 214 forks source link

fix compatibility with scikit-learn by dropping dependency from sklearn.testing #158

Closed StrikerRUS closed 3 years ago

StrikerRUS commented 3 years ago

Fixed #156.

Use normal asserts and ones from numpy.testing module where normal ones cannot be used (approx, arrays, raises).

StrikerRUS commented 3 years ago

@mblondel Will appreciate your review.

mblondel commented 3 years ago

It's better to never use assert. All the assertions missing from numpy.testing are available in nose.tools.

mblondel commented 3 years ago

It's probably easier to start over. You just need to replace the imports.

StrikerRUS commented 3 years ago

@mblondel

It's better to never use assert.

What makes you think so? Is it just a personal matter of taste or you have some links? Pure asserts are the recommended way in the official nose documentation: https://nose.readthedocs.io/en/latest/writing_tests.html

mblondel commented 3 years ago

The assert_ functions will often show a more meaningful message. For instance, when you replace assert_greater(., .) with assert(. > .), the latter will only tell you that the assertion failed, while the former will also tell you the values.

vene commented 3 years ago

fwiw i think in pytest this is no longer needed and plain asserts give good messages, maybe this changed in nose at some point?

StrikerRUS commented 3 years ago

maybe this changed in nose at some point?

I don't think so... nose is dead. The last commit in master was made more than 5 years ago. https://github.com/nose-devs/nose/commits/master

In all modern testing utilities pure asserts is a recommended way, but nose stuck in very old Py2 world. So I guess it might really have less informative output for assert statements comparing to unittest's assert_* functions.

@mblondel Will you consider switching from nose to pytest?

FYI, I'm currently working on improving CI by adding new Python versions #161 and nose doesn't see tests starting from Python 3.8. Here is what I've found so far: https://forum.learncodethehardway.com/t/ex46-nosetests-ran-0-tests-or-ran-1-tests-with-error/3869. 2020 answers are:

That’s so weird. Why in the world would that cause nostests to fail like that? If you are into using the better new testing thing, then replace nose with https://docs.pytest.org/en/latest/

Honestly, the best way to unstuck yourself is by switching to Pytest. You are wasting your time with Nose.

I agree with that. Nose has died since I wrote the book and pytest is just easier.

😃

StrikerRUS commented 3 years ago

@mblondel

Will you consider switching from nose to pytest?

Created #163.

Feel free to close that PR if you'd like to stick to nose.