REF/TYP: implement NDFrameApply #41067
Conversation
jbrockmendel
added
Apply
|
looks fine, cc @rhshadrach |
| # Caller is responsible for checking isinstance(self.f, str) | ||
| f = self.f | ||
| if not isinstance(f, str): | ||
| return None |
There was a problem hiding this comment.
Not necessarily opposed, but why this change? My original thinking was to have each method determine whether or not it would take action rather than the caller - this way that logic is easily shared between the subclasses.
There was a problem hiding this comment.
when going through these and seeing
result = self.foo()
if result is not None:
return result
[...]
it isnt obvious what cases lead to result = None. e.g. before i checked, i imagined it could have been something like
def foo(self):
try:
[...]
except Bar:
return None
There was a problem hiding this comment.
I see. Looking at this now, I see my usage of maybe_* is a bit different than other places in code, where maybe an operation is applied to data but the data is always returned regardless. The downside is that the condition then needs repeated, but it is simple enough (at least at this point) that I don't find that concerning at all.
There was a problem hiding this comment.
I think this change should be applied to maybe_apply_multiple, and the maybe prefix should be dropped. I'm happy to take care of those in a followup.
| # Caller is responsible for checking isinstance(self.f, str) | ||
| f = self.f | ||
| if not isinstance(f, str): | ||
| return None |
There was a problem hiding this comment.
I see. Looking at this now, I see my usage of maybe_* is a bit different than other places in code, where maybe an operation is applied to data but the data is always returned regardless. The downside is that the condition then needs repeated, but it is simple enough (at least at this point) that I don't find that concerning at all.
| # Caller is responsible for checking isinstance(self.f, str) | ||
| f = self.f | ||
| if not isinstance(f, str): | ||
| return None |
There was a problem hiding this comment.
I think this change should be applied to maybe_apply_multiple, and the maybe prefix should be dropped. I'm happy to take care of those in a followup.
|
gentle ping; there's more id like to get to in this area |
if called from GroupByApply or ResamplerWindowApply, these would raise