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

[MRG] Upgrade to support newest scikit-learn version #188

Merged
merged 9 commits into from
Aug 11, 2021
Merged

[MRG] Upgrade to support newest scikit-learn version #188

merged 9 commits into from
Aug 11, 2021

Conversation

ariapoy
Copy link
Collaborator

@ariapoy ariapoy commented Aug 2, 2021

Reference Issues/PRs

No.

What does this implement/fix? Explain your changes.

When upgrading to sklearn==0.24.2, it occurs five FAILs during python setup.py test.

  • Check why test FAILs occurs.
  • Update the test case to pass it.

Any other comments?

  • It's worth thinking about the efficient ways to upgrade when dependencies upgrade.
  • Please see the comments below.

requirements.txt Outdated
@@ -1,7 +1,7 @@
setuptools
numpy
scipy
scikit-learn<=0.19.2
scikit-learn
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should set the version to the earliest version that our CI can pass. It appears to me it should be something like scikit-learn >= 0.23.0? Will 0.22.0 work?

Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yangarbiter

We only set scikit-learn >= 0.24.

There is a conflict between 0.22---0.23 and 0.24 due to cluster.KMeans change.

PS: From 0.23 to 0.24 is a big change. Most APIs are refactored. I wonder maybe they miss something during upgrading.

@yangarbiter
Copy link
Collaborator

Hi @ariapoy,

Thank you so much for submitting this PR.

There is one thing I would like to make sure, the change of test_hs_active_selecting, test_hs_active_selecting, and test_hs_random_selecting is because of Fixed a bug in datasets.load_iris which had two wrong data points, right?

Thanks.
Yao-Yuan

@ariapoy
Copy link
Collaborator Author

ariapoy commented Aug 4, 2021

Hi @yangarbiter,

Yes! Change of sklearn.dataset.load_iris affect test_hs_active_selecting, test_hs_active_selecting, and test_hs_random_selecting in sklearn==0.20.

libact/query_strategies/multiclass/mdsp.py Outdated Show resolved Hide resolved
Change the import style.

Co-authored-by: yangarbiter <yangarbiter@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2021

Codecov Report

Merging #188 (35e82ae) into master (2432b51) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   89.47%   89.60%   +0.12%     
==========================================
  Files          37       37              
  Lines        1568     1597      +29     
==========================================
+ Hits         1403     1431      +28     
- Misses        165      166       +1     
Impacted Files Coverage Δ
libact/query_strategies/multiclass/mdsp.py 78.76% <100.00%> (ø)
libact/utils/__init__.py 77.27% <0.00%> (-4.55%) ⬇️
libact/labelers/ideal_labeler.py 100.00% <0.00%> (ø)
libact/query_strategies/random_sampling.py 100.00% <0.00%> (ø)
...query_strategies/multilabel/binary_minimization.py 100.00% <0.00%> (ø)
..._strategies/multilabel/adaptive_active_learning.py 100.00% <0.00%> (ø)
.../multiclass/active_learning_with_cost_embedding.py 100.00% <0.00%> (ø)
...trategies/density_weighted_uncertainty_sampling.py 98.71% <0.00%> (+0.01%) ⬆️
..._strategies/multilabel/maximum_margin_reduction.py 97.82% <0.00%> (+0.04%) ⬆️
...ery_strategies/multiclass/hierarchical_sampling.py 95.90% <0.00%> (+0.04%) ⬆️
... and 10 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 2432b51...35e82ae. Read the comment docs.

@yangarbiter
Copy link
Collaborator

https://github.com/ntucllab/libact/pull/188/checks?check_run_id=3258702214
It appears that sklearn > 0.24 is not installable on python 2.7, maybe we should also consider dropping support for python 2.7.

@ariapoy
Copy link
Collaborator Author

ariapoy commented Aug 6, 2021

https://github.com/ntucllab/libact/pull/188/checks?check_run_id=3258702214
It appears that sklearn > 0.24 is not installable on python 2.7, maybe we should also consider dropping support for python 2.7.

@yangarbiter
Agree! Follow Warning Scikit-learn 0.20 was the last version to support Python 2.7.
We could drop support for Python 2.7.

Sorry for forgetting to notify you that I just test Ubuntu:20.04 with default Python 3.8.
I notice the FAIL in ubuntu-latest, 3.9 during CI checking.
I'll check it later.

Thanks!

@yangarbiter
Copy link
Collaborator

https://github.com/ntucllab/libact/pull/188/checks?check_run_id=3258702214
It appears that sklearn > 0.24 is not installable on python 2.7, maybe we should also consider dropping support for python 2.7.

@yangarbiter
Agree! Follow Warning Scikit-learn 0.20 was the last version to support Python 2.7.
We could drop support for Python 2.7.

Sorry for forgetting to notify you that I just test Ubuntu:20.04 with default Python 3.8.
Does it work?

I notice the FAIL in ubuntu-latest, 3.9 during CI checking.
I'll check it later.

Thanks!

Libact linting / build (ubuntu-latest, 3.9) (pull_request) this one should fail, which is fine since this is coding style check.
I'll try to fix them (you could also help if you want) after this PR is merged.

@ariapoy ariapoy merged commit 1079085 into ntucllab:master Aug 11, 2021
@ariapoy ariapoy deleted the sklearn-dev branch August 11, 2021 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants