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

Index to grouped data frame using Dicts #2281

Merged
merged 17 commits into from
Aug 29, 2020

Conversation

pdeffebach
Copy link
Contributor

Following discussion in #2265, this is the better strategy.

This PR so far has the implementation and docs, but no tests.

@bkamins
Copy link
Member

bkamins commented Jun 11, 2020

Thank you for the fix. The design looks good to me. I have left some comments. Also function Base.to_index(gd::GroupedDataFrame, idxs::AbstractVector{T}) where {T} needs to be updated to allow Dict.

@bkamins
Copy link
Member

bkamins commented Jun 11, 2020

As I have commented earlier this https://github.com/JuliaData/DataFrames.jl/pull/2281/files#diff-6349421a054bb7e74b7f27ae86304cbaR342 also should be fixed.

@pdeffebach
Copy link
Contributor Author

Sorry about that. Was fixing them but got distracted.

Should be ready for another review. I added a Dict method where the keys are Strings by default. Then I added a bunch of tests.

test/grouping.jl Outdated Show resolved Hide resolved
test/grouping.jl Outdated Show resolved Hide resolved
@bkamins
Copy link
Member

bkamins commented Jun 18, 2020

I have pushed some small things. If CI passes can you please have a final look at it and confirm all is clear and works as intended? Then I think we can approve it (but I will wait with merging for @nalimilan as he has not commented on this API change).

@bkamins bkamins added the non-breaking The proposed change is not breaking label Jun 18, 2020
@bkamins bkamins added this to the 1.0 milestone Jun 18, 2020
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Sorry for the slow reaction. Looks mostly good.

src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved
src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved
src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved
src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved
src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved
src/groupeddataframe/splitapplycombine.jl Outdated Show resolved Hide resolved
test/grouping.jl Outdated Show resolved Hide resolved
@pdeffebach
Copy link
Contributor Author

I've added AbstractDict for this, but am waiting on #2298 for the String vs Symbol output.

@bkamins
Copy link
Member

bkamins commented Aug 20, 2020

CI still fails - can you please check the PR on Julia 1.0 and on Julia nightly to check what causes a problem?

Thank you!

@pdeffebach
Copy link
Contributor Author

Works now!

@bkamins
Copy link
Member

bkamins commented Aug 20, 2020

Bravo! 😄

@bkamins
Copy link
Member

bkamins commented Aug 20, 2020

OK - so can you please just merge it with master and add NEWS.md entry please?

@nalimilan - if you do not comment on this I am going to merge this PR after @pdeffebach adds NEWS.md entry and then work on implementing the conclusions from #2305 including handling Dict.

@pdeffebach
Copy link
Contributor Author

That was a lot easier than a rebase! I think it's ready.

@bkamins
Copy link
Member

bkamins commented Aug 21, 2020

I have resolved merge conflict.

NEWS.md Outdated Show resolved Hide resolved
@bkamins
Copy link
Member

bkamins commented Aug 21, 2020

Looks good to merge.

@nalimilan - can you please have a quick look? I would merge this after CI passes (probably tomorrow as we have a lot of CI runs queued).

src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved
src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved
src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved
src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved
src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved
@bkamins
Copy link
Member

bkamins commented Aug 28, 2020

bump 😄 (I would finish it not to have constantly merge it with master)

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@pdeffebach
Copy link
Contributor Author

Thanks for pinging me on this! Slipped through. Thanks for the feedback Milan.

@bkamins
Copy link
Member

bkamins commented Aug 28, 2020

also merge conflicts need resolving

@pdeffebach
Copy link
Contributor Author

Conflict was just News.md. This can be merged.

@bkamins bkamins merged commit d156320 into JuliaData:master Aug 29, 2020
@bkamins
Copy link
Member

bkamins commented Aug 29, 2020

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-breaking The proposed change is not breaking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants