Skip to content

TYP: annotate Block/BlockManager putmask #32769

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Mar 22, 2020
Merged

TYP: annotate Block/BlockManager putmask #32769

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
66a00fe
TYP: annotate Block/BlockManager.putmask
jbrockmendel Mar 17, 2020
a8e79a9
Merge branch 'master' of https://github.com/pandas-dev/pandas into pu…
jbrockmendel Mar 17, 2020
3fcc6e5
Update pandas/core/internals/blocks.py
jbrockmendel Mar 18, 2020
0f9f370
Merge branch 'master' of https://github.com/pandas-dev/pandas into pu…
jbrockmendel Mar 19, 2020
2c3ae92
update docstrings
jbrockmendel Mar 19, 2020
acf58ad
Merge branch 'master' of https://github.com/pandas-dev/pandas into pu…
jbrockmendel Mar 21, 2020
4c96eba
Merge branch 'putmask-typ' of github.com:jbrockmendel/pandas into put…
jbrockmendel Mar 21, 2020
e30dc86
Merge branch 'master' of https://github.com/pandas-dev/pandas into pu…
jbrockmendel Mar 22, 2020
67ca8f5
merge fixup
jbrockmendel Mar 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions pandas/core/generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8654,12 +8654,7 @@ def _where(

self._check_inplace_setting(other)
new_data = self._data.putmask(
mask=cond,
new=other,
align=align,
inplace=True,
axis=block_axis,
transpose=self._AXIS_REVERSED,
mask=cond, new=other, align=align, axis=block_axis,
)
self._update_inplace(new_data)

Expand Down
38 changes: 24 additions & 14 deletions pandas/core/internals/blocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,26 +925,28 @@ def putmask(
inplace: bool = False,
axis: int = 0,
transpose: bool = False,
):
) -> List["Block"]:
"""
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can return a list with a single block? if so, i think block(s) is more appropriate


Parameters
----------
mask : the condition to respect
mask : the condition to respect
new : a ndarray/object
align : boolean, perform alignment on other/cond, default is True
inplace : perform inplace modification, default is False
align : bool, default True
Perform alignment on other/cond.
inplace : bool, default False
Perform inplace modification.
axis : int
transpose : boolean
Set to True if self is stored with axes reversed
transpose : bool, default False.
Set to True if self is stored with axes reversed.

Returns
-------
a list of new blocks, the result of the putmask
List[Block]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Suggested change
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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't use google style anywhere, this is quite descriptive.

"""
new_values = self.values if inplace else self.values.copy()

Expand Down Expand Up @@ -1670,8 +1672,14 @@ def set(self, locs, values, check=False):
self.values = values

def putmask(
self, mask, new, align=True, inplace=False, axis=0, transpose=False,
):
self,
mask,
new,
align: bool = True,
inplace: bool = False,
axis: int = 0,
transpose: bool = False,
) -> List["Block"]:
"""
putmask the data to the block; we must be a single block and not
generate other blocks
Expand All @@ -1680,14 +1688,16 @@ def putmask(

Parameters
----------
mask : the condition to respect
mask : the condition to respect
new : a ndarray/object
align : boolean, perform alignment on other/cond, default is True
inplace : perform inplace modification, default is False
align : bool, default True.
Perform alignment on other/cond.
inplace : bool, default False.
Perform inplace modification.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally reasonable, I rescind my request.

Used the see-parent-class-docstring pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Returns
-------
a new block, the result of the putmask
List[Block]
"""
inplace = validate_bool_kwarg(inplace, "inplace")

Expand Down
16 changes: 14 additions & 2 deletions pandas/core/internals/managers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,8 +566,20 @@ def where(self, **kwargs) -> "BlockManager":
def setitem(self, indexer, value) -> "BlockManager":
return self.apply("setitem", indexer=indexer, value=value)

def putmask(self, **kwargs):
return self.apply("putmask", **kwargs)
def putmask(
self, mask, new, align: bool = True, axis: int = 0,
):
transpose = self.ndim == 2

return self.apply(
"putmask",
mask=mask,
new=new,
align=align,
inplace=True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How feasible is it to align the inplace arg with blocks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block.putmask is called from other Block methods with inplace=False, so non-trivial

axis=axis,
transpose=transpose,
)

def diff(self, n: int, axis: int) -> "BlockManager":
return self.apply("diff", n=n, axis=axis)
Expand Down
2 changes: 1 addition & 1 deletion pandas/core/series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2798,7 +2798,7 @@ def update(self, other) -> None:
other = other.reindex_like(self)
mask = notna(other)

self._data = self._data.putmask(mask=mask, new=other, inplace=True)
self._data = self._data.putmask(mask=mask, new=other)
self._maybe_update_cacher()

# ----------------------------------------------------------------------
Expand Down