CLN: Remove unused variables #21986

mroeschke · 2018-07-20T04:59:05Z

Breaking up #21974.

Removes non-noqa, seemingly non-controversial, unused local variables according to PyCharm. These are mostly redefined elsewhere or not used.

I added some TODO comments about other unused local variables that seem misused.

gfyoung · 2018-07-20T06:51:42Z

pandas/core/generic.py

@@ -1024,6 +1024,7 @@ def rename(self, *args, **kwargs):
        level = kwargs.pop('level', None)
        axis = kwargs.pop('axis', None)
        if axis is not None:
+            # TODO: axis is unused, is this just validating?
            axis = self._get_axis_number(axis)


I suspect the answer is yes, in which case, not assigning the variable is fine.

gfyoung · 2018-07-20T06:52:41Z

pandas/io/json/json.py

@@ -547,6 +547,7 @@ def _get_object_parser(self, json):

        if typ == 'series' or obj is None:
            if not isinstance(dtype, bool):
+                # TODO: dtype is unused. Should this be an update on kwargs?
                dtype = dict(data=dtype)


If we can find an example that breaks because of this unused variable, that would be good for another PR.

FYI, comment applies to many of the other variables in which you questioned whether it actually should be used, but previous authors didn't get around to implementing functionality with it.

gfyoung · 2018-07-20T06:58:14Z

pandas/core/nanops.py

@@ -479,6 +479,9 @@ def nanvar(values, axis=None, skipna=True, ddof=1):

 @disallow('M8', 'm8')
 def nansem(values, axis=None, skipna=True, ddof=1):
+    # This checks if non-numeric-like data is passed with numeric_only=False
+    # and raises a TypeError otherwise
+    var = nanvar(values, axis, skipna, ddof=ddof)


If var isn't used, don't assign the result of nanvar to a variable. Just call it without.

jreback

rather than comments I would just like to see if changing breaks things. it might be that we are not testing that case or its unused code

jreback · 2018-07-20T12:15:42Z

pandas/core/indexes/base.py

@@ -979,6 +979,7 @@ def __copy__(self, **kwargs):
        return self.copy(**kwargs)

    def __deepcopy__(self, memo=None):
+        # TODO: memo is unused


this is a standard signature, so maybe add a doc-string, you don't need to pass thru memo

jreback · 2018-07-20T12:16:25Z

pandas/core/nanops.py

@@ -635,7 +637,6 @@ def nankurt(values, axis=None, skipna=True):
        adj = 3 * (count - 1) ** 2 / ((count - 2) * (count - 3))
        numer = count * (count + 1) * (count - 1) * m4
        denom = (count - 2) * (count - 3) * m2**2
-        result = numer / denom - adj


huh? is not used?

is this not used?

jreback · 2018-07-20T12:16:40Z

pandas/core/series.py

@@ -2479,6 +2478,7 @@ def sort_values(self, axis=0, ascending=True, inplace=False,
        dtype: object
        """
        inplace = validate_bool_kwarg(inplace, 'inplace')
+        # TODO: axis is unused, is this just validation?


these are checked, so you don't need to assign

jreback · 2018-07-20T12:16:48Z

pandas/core/series.py

@@ -2651,6 +2651,7 @@ def sort_index(self, axis=0, level=None, ascending=True, inplace=False,
        # TODO: this can be combined with DataFrame.sort_index impl as
        # almost identical
        inplace = validate_bool_kwarg(inplace, 'inplace')


same with all of these

jreback · 2018-07-20T12:17:08Z

pandas/io/formats/format.py

@@ -496,6 +496,8 @@ def _chk_truncate(self):
            self.tr_col_num = col_num
        if truncate_v:
            if max_rows_adj == 0:
+                # TODO: Should the next block be elif? row_num here will


this is fine

codecov · 2018-07-20T23:04:10Z

Codecov Report

Merging #21986 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #21986      +/-   ##
==========================================
+ Coverage   92.06%   92.06%   +<.01%     
==========================================
  Files         170      170              
  Lines       50705    50689      -16     
==========================================
- Hits        46680    46668      -12     
+ Misses       4025     4021       -4

Flag	Coverage Δ
#multiple	`90.47% <100%> (ø)`	⬆️
#single	`42.31% <25%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/arrays/interval.py	`92.33% <ø> (-0.03%)`	⬇️
pandas/core/indexes/interval.py	`94.11% <ø> (-0.02%)`	⬇️
pandas/core/indexes/category.py	`97.28% <ø> (ø)`	⬆️
pandas/io/formats/terminal.py	`21.25% <ø> (+0.26%)`	⬆️
pandas/core/arrays/categorical.py	`95.95% <ø> (-0.01%)`	⬇️
pandas/plotting/_timeseries.py	`65.28% <ø> (+0.33%)`	⬆️
pandas/io/formats/html.py	`88.81% <ø> (-0.04%)`	⬇️
pandas/core/groupby/ops.py	`96.57% <ø> (+0.21%)`	⬆️
pandas/core/indexes/base.py	`96.37% <ø> (ø)`	⬆️
pandas/core/internals/blocks.py	`94.45% <ø> (-0.02%)`	⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b7a08b...05e4a36. Read the comment docs.

jbrockmendel · 2018-07-22T16:43:17Z

pandas/core/indexes/category.py

@@ -133,7 +133,7 @@ def _create_from_codes(self, codes, categories=None, ordered=None,
        if name is None:
            name = self.name
        cat = Categorical.from_codes(codes, categories=categories,
-                                     ordered=self.ordered)
+                                     ordered=ordered)


We’re there cases before where the wrong thing was passed? I.e. None != ordered != self.ordered?

is this tested anywhere? I agree this should have broken things

It appears that all instances of this private method _create_from_codes in the codebase never passed an arg to ordered (i.e. ordered always defaulted to None which always got reassigned to self.ordered here)

(pandas-dev) matthewroeschke:pandas-mroeschke matthewroeschke$ grep -R --include="*.py" _create_from_codes . ./pandas/core/indexes/category.py: def _create_from_codes(self, codes, categories=None, ordered=None, ./pandas/core/indexes/category.py: new_target = self._create_from_codes(codes) ./pandas/core/indexes/category.py: return self._create_from_codes(taken) ./pandas/core/indexes/category.py: return self._create_from_codes(np.delete(self.codes, loc)) ./pandas/core/indexes/category.py: return self._create_from_codes(codes) ./pandas/core/indexes/category.py: result = self._create_from_codes(codes, name=name) ./pandas/core/indexes/category.py: # if name is None, _create_from_codes sets self.name

jbrockmendel · 2018-07-22T16:43:55Z

pandas/core/series.py

@@ -2479,7 +2478,8 @@ def sort_values(self, axis=0, ascending=True, inplace=False,
        dtype: object
        """
        inplace = validate_bool_kwarg(inplace, 'inplace')
-        axis = self._get_axis_number(axis)
+        # Valida the axis parameter


Validate typo

jbrockmendel · 2018-07-22T16:45:21Z

Mostly looks good. There are a few places where missing arguments are added that may merit tests.

jreback · 2018-07-24T00:12:55Z

can you rebase

jreback · 2018-07-25T10:03:27Z

pandas/core/indexes/category.py

@@ -133,7 +133,7 @@ def _create_from_codes(self, codes, categories=None, ordered=None,
        if name is None:
            name = self.name
        cat = Categorical.from_codes(codes, categories=categories,
-                                     ordered=self.ordered)
+                                     ordered=ordered)


is this tested anywhere? I agree this should have broken things

mroeschke · 2018-07-26T00:30:55Z

pandas/core/internals/blocks.py

@@ -1248,7 +1248,7 @@ def take_nd(self, indexer, axis, new_mgr_locs=None, fill_tuple=None):
        if fill_tuple is None:
            fill_value = self.fill_value
            new_values = algos.take_nd(values, indexer, axis=axis,
-                                       allow_fill=False)
+                                       allow_fill=False, fill_value=fill_value)


This modification is deep within the internals (block's definition of take_nd), and I am not sure how to really add a test for this.

this actually isn't used since allow_fill=False

mroeschke · 2018-07-26T00:32:35Z

pandas/tests/io/json/test_pandas.py

@@ -642,6 +642,13 @@ def test_series_from_json_precise_float(self):
        result = read_json(s.to_json(), typ='series', precise_float=True)
        assert_series_equal(result, s, check_index_type=False)

+    def test_series_with_dtype(self):


This test should hit the modification made here: https://github.com/mroeschke/pandas/blob/0d7f07783ad9a42bb3fc7f0e3dda0c01b877fe57/pandas/io/json/json.py#L550

mroeschke · 2018-07-26T00:36:31Z

I think everything should generally be covered with a test besides the take_nd modification explained above.

jreback

minor questions, rebase and ping on green.

mroeschke · 2018-07-29T07:47:36Z

@jreback all green

jreback · 2018-07-29T15:32:54Z

thanks @mroeschke

CLN: Remove unused variables

966d79a

mroeschke added the Clean label Jul 20, 2018

gfyoung reviewed Jul 20, 2018

View reviewed changes

Add back variable that was a check

a154bf5

gfyoung reviewed Jul 20, 2018

View reviewed changes

jreback requested changes Jul 20, 2018

View reviewed changes

Remove unnecessary vars and test

b908aff

add memo docstring

234984a

jbrockmendel reviewed Jul 22, 2018

View reviewed changes

Matt Roeschke added 2 commits July 24, 2018 13:39

Merge remote-tracking branch 'upstream/master' into remove_variables

82683e5

Misspelled Validate

d5ddbc1

jreback requested changes Jul 25, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Jul 25, 2018

Add test for modified behavior

0d7f077

mroeschke commented Jul 26, 2018

View reviewed changes

Matt Roeschke added 2 commits July 26, 2018 09:48

Use np.int64 for windows

8492e93

Merge remote-tracking branch 'upstream/master' into remove_variables

8a4f531

jreback approved these changes Jul 28, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into remove_variables

05e4a36

jreback merged commit 0c58a82 into pandas-dev:master Jul 29, 2018

mroeschke deleted the remove_variables branch July 29, 2018 18:16

mroeschke mentioned this pull request Jul 29, 2018

CLN: Unused varables pt2 #22115

Merged

1 task

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

CLN: Remove unused variables (pandas-dev#21986)

ad26dd4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: Remove unused variables #21986

CLN: Remove unused variables #21986

mroeschke commented Jul 20, 2018

gfyoung Jul 20, 2018

gfyoung Jul 20, 2018

gfyoung Jul 20, 2018

gfyoung Jul 20, 2018

jreback left a comment

jreback Jul 20, 2018

jreback Jul 20, 2018

jreback Jul 28, 2018

jreback Jul 20, 2018

jreback Jul 20, 2018

jreback Jul 20, 2018

codecov bot commented Jul 20, 2018 •

edited

Loading

jbrockmendel Jul 22, 2018

jreback Jul 25, 2018

mroeschke Jul 25, 2018

mroeschke Jul 25, 2018

jbrockmendel Jul 22, 2018

jbrockmendel commented Jul 22, 2018

jreback commented Jul 24, 2018

jreback Jul 25, 2018

mroeschke Jul 26, 2018

jreback Jul 28, 2018

mroeschke Jul 26, 2018

mroeschke commented Jul 26, 2018

jreback left a comment

mroeschke commented Jul 29, 2018

jreback commented Jul 29, 2018

CLN: Remove unused variables #21986

CLN: Remove unused variables #21986

Conversation

mroeschke commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 20, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Jul 22, 2018

jreback commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroeschke commented Jul 26, 2018

jreback left a comment

Choose a reason for hiding this comment

mroeschke commented Jul 29, 2018

jreback commented Jul 29, 2018

codecov bot commented Jul 20, 2018 •

edited

Loading