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

Don't freeze version of packages #49

Closed
alxkolm opened this issue Jul 23, 2017 · 7 comments
Closed

Don't freeze version of packages #49

alxkolm opened this issue Jul 23, 2017 · 7 comments

Comments

@alxkolm
Copy link

alxkolm commented Jul 23, 2017

Please, don't freeze versions of packages

'numpy==1.12.1',
'scikit-learn==0.18.1',
'scipy==0.19.0'
@nicodv
Copy link
Owner

nicodv commented Jul 23, 2017

After reading https://blog.miguelgrinberg.com/post/the-package-dependency-blues, I've moved to ranges of dependency package versions:

install_requires=[
        'numpy>=1.13.0, <1.14.0',
        'scikit-learn>=0.18.0, <0.19.0',
        'scipy>=0.19.0, <0.20.0',
    ]

@nicodv nicodv closed this as completed Jul 23, 2017
nicodv added a commit that referenced this issue Jul 23, 2017
@nicodv nicodv mentioned this issue Nov 15, 2017
@shapiromatron
Copy link

scikit-learn has a few more recent versions, could this range be increased?

@nicodv
Copy link
Owner

nicodv commented Jan 25, 2019

@shapiromatron Unfortunately, scikit-learn 0.20 introduces some breaking changes to k-modes. I'm holding off for a bit, but feel free to try it and file a PR. :)

@shapiromatron
Copy link

Thanks @nicodv; I just wanted to make sure it really was required. I'll take a look at see what I can do....

@trevorstephens
Copy link
Contributor

What breaks Nico? gplearn hasn't seen any issues since the RNG changed in I think 0.18 and I believe we mostly use the same sklearn utils

@nicodv
Copy link
Owner

nicodv commented Jan 29, 2019

I seem to recall the sklearn compatibility tests have to be rewritten, @trevorstephens : https://github.com/nicodv/kmodes/blob/master/kmodes/tests/test_common.py

I've always had to customize that file entirely, because kprototypes takes an extra argument (categoricals) to the fit/predict methods and that breaks sklearn compatibility. I'd still like to comply as much as I can, though.

@trevorstephens
Copy link
Contributor

Ah yes that makes sense. I saw something vaguely similar from Py-earth in the mailing list saying some of his tests failed due to extra args as well:

I'm working on updating py-earth for some recent changes in scikit-learn and cython. It seems like check_estimator has been significantly improved, and I'm working through making py-earth compliant with it. I've hit the following issue, though. It seems check_estimator tests score_samples using only X as an argument, and py-earth's score_samples requires y as well. So, my question is: must score_samples work with just X (and therefore maybe I should just remove it from py-earth) or is it okay to have a score_samples that requires y, and I should try to find a workaround for check_estimator?

You might want to pile onto this PR with some suggestions for your package as was suggested for py-earth: scikit-learn/scikit-learn#8022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants