-
Notifications
You must be signed in to change notification settings - Fork 370
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
rebased version for #2327 add filter subsetting to docs #2598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nalimilan - looks good to me (except for comments), but please kindly review it from the language perspective. I would merge it as is, although soon we should add the information about subset
to it.
We have just merged #2496. I will amend your PR with an example how it can be used. |
I have proposed something (subject to correction most likely as I am not a native speaker). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay!
@@ -877,6 +877,58 @@ package provides interfaces similar to LINQ and [dplyr](https://dplyr.tidyverse. | |||
|
|||
See the [Data manipulation frameworks](@ref) section for more information. | |||
|
|||
#### Selecting rows with `filter` and `subset` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd put this section above the one presenting select
and transform
, which is more complex and goes beyond taking subsets. (Transformations probably deserve a separate section so that we only present columns selection, but that's another story.)
That way you don't need to repeat the example: you can just continue with the ones given at the end of the Indexing section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot, as subset
has the same syntax as select
so we need to introduce it after filter
. We could split the discussion of filter
and subset
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah indeed. Let's keep it that way then. But "Taking a Subset" isn't actually a good name for the parent section as it also shows complex transformations. Maybe we should just split each sub-section into its own section, i.e. "Indexing", "Selecting and transforming columns" and "Selecting rows".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
1 │ 3 Claire c | ||
``` | ||
|
||
An alternative formulation, which notably saves on the need to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we provide subset
with a syntax closer to select
, I'm not sure we should mention filter
at all in Getting Started. Or maybe just mention it at the end for reference. What do you think @bkamins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do this - then it should be after select
section as commented above. So - should I make this change?
for which all transformations produce `true` are inluded in the result: | ||
|
||
```jldoctest dataframe | ||
julia> subset(df, :x => ByRow(>(2)), :a => ByRow(==('c'))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe start showing the standard x -> x > 2
syntax? That's easier to grasp for newcomers, and more general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - I will update it when we settle on the general layout.
Co-authored-by: Milan Bouchet-Valat <[email protected]>
@floswald - would you want to update this PR to take into account the current state of the documentation? Thank you! |
closed given #2900 is merged (and anything here would have to be heavily rewritten so probably it is better to open an new PR if needed). Thank you for raising the issue. |
rebased try!