Skip to content

Conversation

ExpandingMan
Copy link
Contributor

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
Copy link
Owner

cstjean commented Nov 17, 2020

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
Copy link
Contributor Author

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
Copy link

RDatasets 0.7.2 now supports DataFrames 0.22.

@cstjean
Copy link
Owner

cstjean commented Nov 18, 2020

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
Copy link
Contributor Author

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.

@@ -8,7 +8,7 @@ env:
- PYTHON=Conda
julia:
- 1.0
- 1.4
- 1.5

Choose a reason for hiding this comment

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

Why not just use 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I keep forgetting that, I only recently realized that putting 1 gives latest 1. Let's see what the test result is and I'll put it in.

@ExpandingMan
Copy link
Contributor Author

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

@cstjean
Copy link
Owner

cstjean commented Nov 19, 2020

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
Copy link

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
Copy link
Contributor Author

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

@nalimilan
Copy link

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 davidavdav/GaussianMixtures.jl#68.

@nalimilan
Copy link

Though you could try tweaking only the Compat bounds instead.

@ExpandingMan
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Owner

cstjean commented Dec 2, 2020

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
Copy link
Contributor Author

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
Copy link
Owner

cstjean commented Dec 2, 2020

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
Copy link
Contributor Author

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
Copy link

JuliaRegistries/General#25016 was opened, but it requires manual merging. Make some noise there, that could help!

@nalimilan
Copy link

See also davidavdav/GaussianMixtures.jl#81.

@nalimilan
Copy link

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

@ExpandingMan
Copy link
Contributor Author

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
Copy link
Contributor Author

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 cstjean merged commit 9f15da6 into cstjean:master Dec 4, 2020
@cstjean
Copy link
Owner

cstjean commented Dec 4, 2020

Thank you for this PR!

@ExpandingMan ExpandingMan deleted the dataframesup branch December 4, 2020 16:44
@ExpandingMan
Copy link
Contributor Author

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

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