search

cstjean / ScikitLearn.jl

Julia implementation of the scikit-learn API https://cstjean.github.io/ScikitLearn.jl/dev/
Other
546 stars 74 forks source link

bumped dataframes version #92

Closed ExpandingMan closed 3 years ago

ExpandingMan commented 3 years ago

Don't think any of the changes should affect this package.

Note that, in the long run, it would probably be best to remove DataFrames as a dependency in favor of the Tables interface, but I didn't look into how involved that would be.

cstjean commented 3 years ago

I don't understand why https://travis-ci.org/github/cstjean/ScikitLearn.jl/jobs/744241946#L188 says v.0.22.0, but https://travis-ci.org/github/cstjean/ScikitLearn.jl/jobs/744241946#L296 says 0.21.8. And I don't get why it fails on 1.4 but succeeds on nightly 😢

ExpandingMan commented 3 years ago

Gah, this stuff is so annoying. Presumably will need to at least wait for this. Particularly aggravating because it's only used in tests.

nalimilan commented 3 years ago

RDatasets 0.7.2 now supports DataFrames 0.22.

cstjean commented 3 years ago

Ok, so there's some df[:some_col] somewhere in the code causing the failures, that should be fixed. What mystifies me is that the nightly tests pass, but that's probably because the resolver installed some old version of DataFrames.jl.

ExpandingMan commented 3 years ago

Ok, I found the failure and fixed it. I've also taken the liberty to switch the latest travis run from 1.4 to 1.5. Let's see if it passes.

ExpandingMan commented 3 years ago

There was a failure on mac. Looks full-blown dependency hell involving GaussianMixtures and StatsBase

cstjean commented 3 years ago

Offf, I don't know what the right solution is to this. GaussianMixtures is just a test dependency, so technically, 1.0 might still work perfectly well. Unless someone has a bright idea, I'd say we drop Julia 1.0 support in the name of sanity. 1.6 will be LTS anyway, right?

nalimilan commented 3 years ago

Looks like the resolver got lost in the requirements on Compat imposed by GaussianMixtures and ScikitLearn. Maybe you could try requiring Compat = "3" to make things simpler for it. Or even Compat = "3.6" like GaussianMixtures. Likewise, requiring StatsBase = "0.33" might help. Of course that's not ideal...

ExpandingMan commented 3 years ago

Alright, I tried that, let's see how it goes.

nalimilan commented 3 years ago

Failures are due to GaussianMixtures not supporting StatsBase 0.33. It supports it on master but no release has been tagged since then. I've asked at https://github.com/davidavdav/GaussianMixtures.jl/pull/68.

nalimilan commented 3 years ago

Though you could try tweaking only the Compat bounds instead.

ExpandingMan commented 3 years ago

Ok, I tried triggering this again (by bumping the PyCall version) and am just sort of hoping that everything miraculously works now because of changes in other packages.

ExpandingMan commented 3 years ago

@cstjean, I don't see much in the way of DataFrames here other than in src/dataframes.jl, is that correct or am I missing some? I'm considering how much work it would be to drop DataFrames as a dependency altogether, and switch to the Tables.jl interface. Would you be open to this? (I don't think it would be breaking, but I'm not sure yet)

cstjean commented 3 years ago

Yes, the DataFrames support should be confined to src/dataframes.jl. If you're willing to do the work to get Tables.jl support working, and you believe that it's not (significantly?) breaking for users, then that sounds good to me.

ExpandingMan commented 3 years ago

I'm not yet confident that it won't be breaking, but I am confident it won't be very breaking. Ideally would like to merge this PR before taking that on, but it's still breaking because GaussianMixtures needs a tag.

cstjean commented 3 years ago

Ideally would like to merge this PR before taking that on, but it's still breaking because GaussianMixtures needs a tag.

If that would help, I would be fine merging this with @test_broken or somesuch temporary work-around.

ExpandingMan commented 3 years ago

It's up to you of course, but there's no good reason why the compat issue can't just be fixed, so maybe let's wait a bit longer. I tried to make noise about this in various places today.

nalimilan commented 3 years ago

https://github.com/JuliaRegistries/General/pull/25016 was opened, but it requires manual merging. Make some noise there, that could help!

nalimilan commented 3 years ago

See also https://github.com/davidavdav/GaussianMixtures.jl/pull/81.

nalimilan commented 3 years ago

OK, GaussianMixtures has been tagged. Should be good to start another CI run.

ExpandingMan commented 3 years ago

Looks like it's passing. For some reason travis now seems to wait for mac to finish before it runs linux, which I find rather irritating.

ExpandingMan commented 3 years ago

Ok, passing, finally. @cstjean , please merge when you get a chance?

Finally I think this is the last major package holding back DataFrames 0.22

cstjean commented 3 years ago

Thank you for this PR!

ExpandingMan commented 3 years ago

No problem! Also, don't forget we'll need a tag. Thanks!