-
Notifications
You must be signed in to change notification settings - Fork 367
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
add filter and filter! to GroupedDataFrame #2279
Conversation
end | ||
|
||
""" | ||
filter!(function, gdf::GroupedDataFrame) |
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.
Do we have any other in-place functions on GroupedDataFrame
already, apart from select!
and transform!
? I'm asking because for consistency with the two latter, it would make sense for filter!
to drop rows from parent(gdf)
. Though that could be a unexpected. We need a general policy on this, which could also apply to in-place oeprations on SubDataFrame
.
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.
This is a good question (actually I initially considered to add filter
only for these reasons). It applies both to filter
and filter!
. I have not thought about it because what I proposed was most natural.
So the considerations are:
- we do not have other functions that mutate
GroupedDataFrame
in-place - we do not have functions that mutate
SubDataFrame
schema in-place (of course we allow mutating the data, but not schema) select!
andtransform!
work only for someGroupedDataFrames
(ones that have no groups dropped) and they guarantee to retain row count in the parent; also it is in general expected (and documented) that they mutate the parent as they define new columns in generalfilter!
andfilter
can work on an already "subsetted"GroupDataFrame
so then it would be confusing what should happen with groups that are already not present.- it is natural to expect that
filter(predicate, collecion)
does the same ascollection[predicate.(collection)]
, which does not mutate the parent ofcollection
if it is present.
All in all I think it is better to keep what we have.
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
I have also pushed updates to docstrings of |
Your comment made me realize that in the past we assumed |
This should be rebased after #2280 is finished and only then merged. |
So the only downside of |
Maybe let's leave it out until we have a clear use case for it? If nobody requests it maybe it's not needed, and that keeps the API more consistent regarding what |
OK - I have removed |
similar - only coverage fails |
Thank you! |
@nalimilan - this is a follow up to your comment in #1732.