Skip to content

CLN: clean up blocks.py #36534

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 3 commits into from
Sep 24, 2020
Merged

CLN: clean up blocks.py #36534

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1b7b84
clean up blocks.py
erfannariman Sep 21, 2020
bcdaa65
revert changes for tests
erfannariman Sep 21, 2020
a51a480
Merge branch 'master' into cln-clean-ups
erfannariman Sep 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: 4 additions & 3 deletions pandas/core/internals/blocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ def _holder(self):

@property
def _consolidate_key(self):
return (self._can_consolidate, self.dtype.name)
return self._can_consolidate, self.dtype.name

@property
def is_view(self) -> bool:
Expand Down Expand Up @@ -1363,6 +1363,7 @@ def where(
errors : str, {'raise', 'ignore'}, default 'raise'
- ``raise`` : allow exceptions to be raised
- ``ignore`` : suppress exceptions. On error return original object
try_cast: bool, default False
axis : int, default 0
Comment on lines +1366 to 1367
Copy link
Member

Choose a reason for hiding this comment

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

What this parameter try_cast is for? Looks like it is not used in the function. It will be better for the code cleanup if you delete it from the function signature and everywhere it gets called.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I saw quite some of these arguments in other functions as well (ones which are not used), but when I remove them, some tests fail. So would it be safe to remove those in the tests or the tests themselves as well?

Copy link
Member

Choose a reason for hiding this comment

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

It is hard to say why tests fail in this case without looking at the traceback.
Need to check whether the test itself calls the method with all the arguments, or the method is called somewhere else. In the latter case one needs to change all calls of the method (that could probably be called in multiple places).

Copy link
Member

Choose a reason for hiding this comment

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

if its just that the tests pass the unused arg directly, then its fine to remove the arg from the test too.

If you've got some easy things (like this PR so far) and some less-obvious things, usually best to keep them as separate PRs.


Returns
Expand Down Expand Up @@ -1633,8 +1634,8 @@ def __init__(self, values, placement, ndim=None):
def shape(self):
# TODO(EA2D): override unnecessary with 2D EAs
if self.ndim == 1:
return ((len(self.values)),)
return (len(self.mgr_locs), len(self.values))
return (len(self.values),)
return len(self.mgr_locs), len(self.values)

def iget(self, col):

Expand Down