Skip to content

Allow passing multiple columns to subset#2926

Merged
bkamins merged 12 commits intomainfrom
bk/flexible_subset
Nov 8, 2021
Merged

Allow passing multiple columns to subset#2926
bkamins merged 12 commits intomainfrom
bk/flexible_subset

Conversation

@bkamins
Copy link
Copy Markdown
Member

@bkamins bkamins commented Nov 3, 2021

Fixes #2924

@bkamins bkamins self-assigned this Nov 3, 2021
@bkamins bkamins added the feature label Nov 3, 2021
@bkamins bkamins added this to the 1.3 milestone Nov 3, 2021
@bkamins bkamins requested a review from nalimilan November 3, 2021 19:00
@bkamins
Copy link
Copy Markdown
Member Author

bkamins commented Nov 5, 2021

@nalimilan - can you please re-review. I have forgotten to add NEWS.md entry + update documentation. I have also added a clear explanation that filter is not recommended and subset should be used instead in the documentation.

@bkamins bkamins linked an issue Nov 6, 2021 that may be closed by this pull request
bkamins and others added 3 commits November 7, 2021 00:19
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@bkamins
Copy link
Copy Markdown
Member Author

bkamins commented Nov 6, 2021

I have updated the docs following your suggestions.

Comment on lines +5 to +6
for working with tabular data. In DataFrames.jl the `DataFrame` and
`SubDataFrame` types are subtypes of `AbstractDataFrame`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd put this after the sentence below which gives the overall definition.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done


This method is defined so that DataFrames.jl implements the Julia API for
collections, but it is generally recommended to use the [`subset!`](@ref)
function instead, as it is both faster and more consistent with other
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually I forgot that you could pass cols here to ensure type stability, and subset isn't faster in that case, right? Maybe we should only mention consistency?

Also, I'd only mention this at the end of the docstring, possibly in a !!! note. Better not add too much noise for people who do want to use that function.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

changed all docstrings for filter and subset and the manual entry.

bkamins and others added 2 commits November 8, 2021 10:19
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@bkamins bkamins merged commit 3e512b4 into main Nov 8, 2021
@bkamins bkamins deleted the bk/flexible_subset branch November 8, 2021 21:54
@bkamins
Copy link
Copy Markdown
Member Author

bkamins commented Nov 8, 2021

Thank you!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve filter docs subset doesn't accept a vector of transformations

2 participants