TYP: annotate Block/BlockManager putmask #32769
Conversation
| mask=mask, | ||
| new=new, | ||
| align=align, | ||
| inplace=True, |
There was a problem hiding this comment.
How feasible is it to align the inplace arg with blocks?
There was a problem hiding this comment.
Block.putmask is called from other Block methods with inplace=False, so non-trivial
Co-Authored-By: Simon Hawkins <[email protected]>
pandas/core/internals/blocks.py
Outdated
| """ | ||
| putmask the data to the block; it is possible that we may create a | ||
| new dtype of block | ||
|
|
||
| return the resulting block(s) | ||
| Return the resulting blocks. |
There was a problem hiding this comment.
can return a list with a single block? if so, i think block(s) is more appropriate
|
|
||
| Returns | ||
| ------- | ||
| a list of new blocks, the result of the putmask | ||
| List[Block] |
There was a problem hiding this comment.
from numpdoc for Returns section...
"Explanation of the returned values and their types. Similar to the Parameters section, except the name of each return value is optional. The type of each return value is always required"
the pandas docstring guide appears to say the same.
The guides do NOT say this section is optional.
so to comply something like...
| List[Block] | |
| list of Block | |
| A list of new blocks, the result of the putmask. |
personally, i'd be happy following the google style here.
"Describe the type and semantics of the return value. If the function only returns None, this section is not required. It may also be omitted if the docstring starts with Returns or Yields (e.g. """Returns row from Bigtable as a tuple of strings.""") and the opening sentence is sufficient to describe return value."
and make the returns section optional for internal docstings in the validation.
There was a problem hiding this comment.
we don't use google style anywhere, this is quite descriptive.
pandas/core/internals/blocks.py
Outdated
| align : bool, default True. | ||
| Perform alignment on other/cond. | ||
| inplace : bool, default False. | ||
| Perform inplace modification. |
There was a problem hiding this comment.
missing axis and transpose parameters and descriptions.
personally, i'd be happy following the google style here...
"A method that overrides a method from a base class may have a simple docstring sending the reader to its overridden method’s docstring, such as """See base class.""". The rationale is that there is no need to repeat in many places documentation that is already present in the base method’s docstring. However, if the overriding method’s behavior is substantially different from the overridden method, or details need to be provided (e.g., documenting additional side effects), a docstring with at least those differences is required on the overriding method."
This docstring appears to be duplicate of Block.putmask with a minor inconsistency. i.e. Return the resulting blocks. vs return the resulting block
There was a problem hiding this comment.
adding the missing axis and transpose params ill do now, but for the rest (here and in other comments), can we not make this the PR where we start implementing the discussed standards? This is preliminary to more-important follow-ups.
There was a problem hiding this comment.
can we not make this the PR where we start implementing the discussed standards
IIUC these are our standards already in existence for changes made in this PR.
As a responsible reviewer, just pointing out where the additions/changes in this PR don't meet those standards.
If you feel that this is an unnecessary distraction or disruptive to workflow, then that could be a valid argument in support for a more lightweight standard for internal docstrings.
There was a problem hiding this comment.
Totally reasonable, I rescind my request.
Used the see-parent-class-docstring pattern.
There was a problem hiding this comment.
If you feel that this is an unnecessary distraction or disruptive to workflow, then that could be a valid argument in support for a more lightweight standard for internal docstrings.
You could also say that this more lightweight standard for internal docstrings already exist: we typically don't review those in such detail on formatting issues.
(and to be clear: I think it is good to have this discussion (but in the other issue), as it is certainly not clear or some kind of unspoken rule)
There was a problem hiding this comment.
You could also say that this more lightweight standard for internal docstrings already exist: we typically don't review those in such detail on formatting issues.
I'm hoping that we can add validation of the internal docstrings to the CI and the CI won't be so lenient.
I think it is good to have this discussion (but in the other issue)
yes. sorry @jbrockmendel
|
@simonjayhawkins i lost track, is there anything left actionable here? |
|
rebased+green |
|
needs another rebase |
|
rebased+green |
|
|
||
| Returns | ||
| ------- | ||
| a list of new blocks, the result of the putmask | ||
| List[Block] |
There was a problem hiding this comment.
we don't use google style anywhere, this is quite descriptive.
|
thanks @jbrockmendel |
medium-term goal is to avoid passing Series/DataFrame objects to Block methods via BlockManager.apply