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.

Sign up for GitHub

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

Jump to bottom

Avoid method dispatch ambiguities in DataFrames.jl #3179

Merged

bkamins merged 9 commits into main from bk/fix_ambiguities

Sep 26, 2022

Member

bkamins commented Sep 25, 2022

@nalimilan - this is a first step towards DataFrames.jl 1.4. release. To make sure things are precompiled correctly one of the recommendations is to avoid dispatch ambiguities. This recommendation makes sense in general.
This PR:

fixes these cases
fixes incorrect behavior that was detected when analyzing these ambiguities
makes sure that in the future we test for dispatch ambiguities and for unbound type parameters

I comment on the reasons for changes in the code.


          avoid method dispatch ambiguities in DataFrames.jl

7ff8e83

bkamins requested a review from nalimilan

September 25, 2022 08:07

bkamins added the bug label

bkamins added this to the 1.4 milestone

bkamins commented

View reviewed changes

src/abstractdataframe/abstractdataframe.jl Show resolved Hide resolved

bkamins commented

View reviewed changes

src/groupeddataframe/fastaggregates.jl Show resolved Hide resolved

bkamins commented

View reviewed changes

src/groupeddataframe/groupeddataframe.jl Show resolved Hide resolved

bkamins commented

View reviewed changes

src/groupeddataframe/groupeddataframe.jl Show resolved Hide resolved

bkamins commented

View reviewed changes

src/groupeddataframe/groupeddataframe.jl Show resolved Hide resolved

bkamins commented

View reviewed changes

src/other/broadcasting.jl Outdated Show resolved Hide resolved

bkamins commented

View reviewed changes

src/other/broadcasting.jl Show resolved Hide resolved


          fix test

a7ffde9

nalimilan reviewed

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

src/groupeddataframe/fastaggregates.jl 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 Show resolved Hide resolved

src/groupeddataframe/groupeddataframe.jl Outdated Show resolved Hide resolved

test/dataframe.jl Show resolved Hide resolved

bkamins and others added 6 commits

September 25, 2022 13:56


          Apply suggestions from code review

9fddcb4

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>


          implement changes following review

69cd600


          improve test coverage


          remove eltype check in check_aggregate

610b5a2


          Update NEWS.md

47c4e4c

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>


          add fixes following review comments

b61f83f

Member Author

bkamins commented Sep 25, 2022

I have updated the PR. The thing to check is the check_agrregate changes I added (I think they are correct, but these things are tricky).

nalimilan approved these changes

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved


          Apply suggestions from code review

2065c75

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins merged commit c3fd232 into main

bkamins deleted the bk/fix_ambiguities branch

September 26, 2022 11:39

Member Author

bkamins commented Sep 26, 2022

Thank you!

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