CLN: Exception*2 in groupby wrapper #28771
Conversation
pandas/core/groupby/groupby.py
Outdated
| except AttributeError: | ||
| # e.g. SparseArray has no flags attr | ||
| raise ValueError | ||
| import inspect |
There was a problem hiding this comment.
import at the top of the module
There was a problem hiding this comment.
its an expensive import im trying to get rid of in the one other place we use it
There was a problem hiding this comment.
What do we consider an "expensive" import? It seems natural to me to import the stdlib at the top of a module so would prefer to do that as well unless there really is a lot to be gained here
There was a problem hiding this comment.
About 2.6 ms (of about 400 ms total). So its not massive, but at this point it is among the lower-hanging fruit. Fine to move I guess.
pandas/core/groupby/groupby.py
Outdated
| except AttributeError: | ||
| # e.g. SparseArray has no flags attr | ||
| raise ValueError | ||
| import inspect |
There was a problem hiding this comment.
What do we consider an "expensive" import? It seems natural to me to import the stdlib at the top of a module so would prefer to do that as well unless there really is a lot to be gained here
pandas/core/groupby/groupby.py
Outdated
| except AttributeError: | ||
| # e.g. SparseArray has no flags attr | ||
| raise ValueError | ||
| sig = inspect.signature(f) |
There was a problem hiding this comment.
Could this be done before the currying? IIUC correctly we are mutating the kwargs after the function has been defined but before it gets executed. I think would be more intuitive if we set up the appropriate kwargs before the currying even happens
There was a problem hiding this comment.
i wasnt sure what the semantics are of editing kwargs. is it necessarily a new dict, or could we be editing a dict that exists in the calling namespace?
There was a problem hiding this comment.
I don't know definitely but I think the same rules would apply to kwargs as to default arguments, i.e. the parameter values get evaluated on function execution and not definition
https://docs.python.org/3/reference/compound_stmts.html#function-definitions
There was a problem hiding this comment.
If there wasn't the if name in base.plotting_methods case just above this I would think the answer is "yes" we can do this earlier, but I'm not confident as-is. Good candidate for follow-up to see if we can get that case out of this wrapper.
| # in tests.groupby.test_function.test_non_cython_api | ||
| raise ValueError | ||
|
|
||
| wrapper.__name__ = name |
There was a problem hiding this comment.
Any reason we don't just use functools.wraps? Maybe a general question in our code base but unclear to me why we choose to assign to the dunders directly instead of using functools
|
not a big deal but can you merge master after @WillAyd refactor. |
There was a problem hiding this comment.
lgtm - these Exception cleanups are great thanks @jbrockmendel
|
rebased+green |
|
Thanks @jbrockmendel |
Gets rid of two
except Exceptions in the same function, which is also one of the more heavily-nested functions in there.