[MRG] added parse_version to sklearn.utils

[dalmia]

Reference Issue

Fixes #7980

What does this implement/fix? Explain your changes.

This adds parse_version to sklearn.utils which is to replace sklearn.utils.fixes._parse_version and LooseVersion in scikit-learn

Any other comments?

I have just followed what @lesteve guided me in the issue thread. Please let me know if my approach is correct and if we can start implementing it throughout scikit-learn.

[dalmia]

ping @lesteve

[lesteve]

You will need to add tests and to use this everywhere that we do version comparison in the code.

[dalmia]

fixes._parse_version wasn't being used anywhere else in the codebase and the output of git grep 'LooseVersion' sklearn/ remains:

sklearn/_build_utils/__init__.py:from distutils.version import LooseVersion
sklearn/_build_utils/__init__.py:if LooseVersion(Cython.__version__) < CYTHON_MIN_VERSION:

[lesteve]

fixes._parse_version wasn't being used anywhere else in the codebase and the output of git grep 'LooseVersion' sklearn/ remains:

Just curious why did you not replace all of them? Maybe you can not import sklearn during the cython build or something like this ?

Quickly grepping for __version__ I see that you still need to replace:

• cmp_version in sklearn/neighbors/tests/test_dist_metrics.py
• _parse_version in sklearn/utils/fixes.py

[dalmia]

Yes, the relative import ..utils import parse_version is not allowed in _build_utils. For some reason, I felt earlier that sklearn/utils/fixes.py should be left unchanged, made the change now. And I had missed out on cmp_version, thank you for pointing that out :)

[dalmia]

Working on the AppVeyor failures. Travis errors seem erroneous. Can you review the Travis failures please?
Thanks.

[codecov-io]

Codecov Report

Merging #8301 into master will decrease coverage by 1.52%.
The diff coverage is 72.08%.

@@            Coverage Diff             @@
##           master    #8301      +/-   ##
==========================================
- Coverage   96.19%   94.66%   -1.53%     
==========================================
  Files         348      344       -4     
  Lines       64645    60968    -3677     
==========================================
- Hits        62187    57718    -4469     
- Misses       2458     3250     +792

Impacted Files	Coverage Δ
sklearn/preprocessing/tests/test_data.py	99.7% <100%> (-0.23%)	⬇️
sklearn/metrics/tests/test_classification.py	99.57% <100%> (-0.43%)	⬇️
sklearn/linear_model/tests/test_logistic.py	99.66% <100%> (-0.34%)	⬇️
sklearn/feature_extraction/tests/test_image.py	5.23% <100%> (-94.77%)	⬇️
sklearn/utils/tests/test_version.py	100% <100%> (ø)
sklearn/utils/tests/test_extmath.py	97.04% <100%> (-2.68%)	⬇️
sklearn/linear_model/omp.py	95.58% <100%> (-0.37%)	⬇️
sklearn/cross_decomposition/pls_.py	96.33% <100%> (-0.4%)	⬇️
sklearn/utils/__init__.py	93.85% <100%> (-0.68%)	⬇️
sklearn/utils/extmath.py	93.97% <100%> (-2.2%)	⬇️
... and 279 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e29334...71635e2. Read the comment docs.

[lesteve]

Travis errors seem erroneous. Can you review the Travis failures please?

Good practice: be explicit. What is the error? What makes you think it is erroneous?

Quickly looking at the Travis error they seems genuine.

[dalmia]

Actually the time when I commented, the errors were appearing in files that I had not changed and were defying class definitions. That's why they seemed erroneous. Look alright now, working on fixing it.

[dalmia]

Any reason the codecov errors should be appearing?

[lesteve]

Any reason the codecov errors should be appearing?

I think we can ignore that for now. I'll try to review your changes in more details in the upcoming days.

For the record, codecov errors means that too much code you have changed or added in your PR is not covered by tests. You can try to install the codecov extension for your favourite browser and look for code that is not covered by tests. You can also try to browse the codecov link but sometimes it is harder to make sense of it. Quickly looking at it it seems that some code in sklearn/utils/version.py (for example the __str__ method or the Infinity classes)

[lesteve]

A more generic question is did you just take the setuptools code as it was or did you change a few things?

To be honest I was hoping the code could be simplified but thinking about it it's probably better to keep it as close as possible to the setuptools code.

[lesteve]

You should add a module docstring here. Among other things, it should mention where (which package, file, version) the code has been taken from.

[lesteve]

Do you understand when Infinity and NegativeInfinity are used? It would be great to add tests for this so that the coverage increases.

[cmarmo]

Thank you @dalmia for your work. I'm closing this as a lot of things have changed and #7980 probably needs a different approach now to be solved.

[cmarmo]

Reopening as per comment.