CLN refactor rest of core #37586

MarcoGorelli · 2020-11-02T14:17:39Z

Some refactorings found by Sourcery https://sourcery.ai/

I've removed the ones of the kind

- if param:
-     var = a
- else:
-     var = b
+ var = a if param else b

jreback

one comment otherwise lgtm

jreback · 2020-11-02T23:19:28Z

pandas/core/generic.py

                else:
-                    object.__setattr__(self, name, value)
+                    self[name] = value


can you check the logic that was changed here (IIRC this is for performance but the change might be good)

I am not sure about this change on L5509. pls revert and can do in a separate PR when checking perf. This causes recursion (which might be fine, though need to confirm)

…core

jbrockmendel · 2020-11-08T00:03:43Z

pandas/core/algorithms.py

+        is_object_dtype(dtype)
+        or not is_object_dtype(dtype)
+        and is_object_dtype(values)
+        and dtype is None


can you put parens where appropriate to make this more obvious

jbrockmendel · 2020-11-08T00:06:24Z

pandas/core/base.py

@@ -1244,8 +1242,7 @@ def searchsorted(self, value, side="left", sorter=None) -> np.ndarray:

    def drop_duplicates(self, keep="first"):
        duplicated = self.duplicated(keep=keep)
-        result = self[np.logical_not(duplicated)]
-        return result
+        return self[np.logical_not(duplicated)]


id prefer ~duplicated over logical_not

jbrockmendel · 2020-11-08T00:10:25Z

pandas/core/generic.py

+            left = left.copy()
+            right = right.copy()
+            left.index = join_index
+            right.index = join_index


identical(?) to chunk above, could make a helper. also im pretty sure mypy is going to complain about Index not having a tz attribute

@jbrockmendel do you know when this code is meant to be reached? Running

pytest pandas/tests/generic/

doesn't cover it. I couldn't get here with test_fast.sh either, even though codecov shows it as covered

Looks like test_alignment_doesnt_change_tz from #36503 hits the other usage of this pattern, might not have gotten both

jbrockmendel · 2020-11-08T00:11:35Z

pandas/core/nanops.py

@@ -185,14 +182,11 @@ def _get_fill_value(
            else:
                return -np.inf
    else:
-        if fill_value_typ is None:
-            return iNaT
+        if fill_value_typ is not None and fill_value_typ == "+inf":


i think can just do the str comparison?

jbrockmendel · 2020-11-08T00:11:56Z

pandas/core/nanops.py

-    if values.ndim == 1:
-        return fill_value
-    elif axis is None:
+    if values.ndim == 1 or axis is None:


i think this is a case where its useful to see we get coverage of both cases

jbrockmendel · 2020-11-08T00:13:38Z

pandas/core/sorting.py

@@ -218,7 +218,7 @@ def decons_group_index(comp_labels, shape):
    x = comp_labels
    for i in reversed(range(len(shape))):
        labels = (x - y) % (factor * shape[i]) // factor
-        np.putmask(labels, comp_labels < 0, -1)
+        np.putmask(labels, x < 0, -1)


lets just do labels[x < 0] = -1, putmask has different semantics when the last arg is a listlike but thats not relevant here

…core

jreback · 2020-11-18T13:54:22Z

can you merge master ping on green

MarcoGorelli · 2020-11-18T15:14:27Z

I still need to address #37586 (comment) , will ping when it's ready

…core

jreback · 2020-11-24T13:46:51Z

moving off 1.2 until fixed up

MarcoGorelli · 2020-11-24T13:56:44Z

Sure, no problem, though I believe I've responded to the review comments, I think it should be ready now

jreback · 2020-12-22T14:13:55Z

@MarcoGorelli can you rebase and see if you can get passing

…core

jreback · 2020-12-22T21:11:42Z

ok this looks fine, but a ci /checks is failing. ping on greenish

…core

jreback

comment + merge master. master is moving, so ping on green when ready.

jreback · 2021-01-03T17:27:51Z

pandas/core/algorithms.py

@@ -114,11 +114,8 @@ def _ensure_data(
        values = extract_array(values, extract_numpy=True)

    # we check some simple dtypes first
-    if is_object_dtype(dtype):
+    if is_object_dtype(dtype) or (is_object_dtype(values) and (dtype is None)):


can you remove the parens around dtype is None as confusing (and actually put around the entire expression and format)

…core

jreback

2 changes to revert. these types of PRs are very hard to review. in the future pls split out the non-controversial changes otherwise these linger forever.

ping on green when reverted

jreback · 2021-01-17T15:43:07Z

pandas/core/generic.py

                else:
-                    object.__setattr__(self, name, value)
+                    self[name] = value


I am not sure about this change on L5509. pls revert and can do in a separate PR when checking perf. This causes recursion (which might be fine, though need to confirm)

jreback · 2021-01-17T15:44:06Z

pandas/core/generic.py

@@ -11106,6 +11091,22 @@ def last_valid_index(self):
        return self._find_valid_index("last")


+def _set_join_index(


this should not be here but in pandas/core/reshape/merge.py or appropriate

…core

jreback · 2021-01-20T01:41:07Z

thanks @MarcoGorelli this is a nice cleanup. in the future pls do these types of changes in smaller batches.

refactor rest of core

3f275aa

jreback added the Code Style Code style, linting, code_checks label Nov 2, 2020

jreback added this to the 1.2 milestone Nov 2, 2020

jreback requested changes Nov 2, 2020

View reviewed changes

MarcoGorelli added 3 commits November 4, 2020 07:44

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

4a6c153

…core

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

9db6e6c

…core

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

8794ceb

…core

jbrockmendel reviewed Nov 8, 2020

View reviewed changes

MarcoGorelli added 7 commits November 8, 2020 08:18

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

803f49b

…core

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

4511a0c

…core

apply some suggestions

9c3598b

logicalnot -> ~

873d430

parens

31d29af

parens

7a51410

parens

a31e89d

MarcoGorelli marked this pull request as draft November 18, 2020 15:14

MarcoGorelli added 3 commits November 23, 2020 17:12

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

a8c8c9c

…core

factor out _set_join_index

5e053d4

🏷️ type

5f0a278

jreback removed this from the 1.2 milestone Nov 24, 2020

MarcoGorelli marked this pull request as ready for review November 24, 2020 13:55

MarcoGorelli added 2 commits December 16, 2020 15:13

avoid duplicated check

7fda4e7

remove redundant check

c36bf8d

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

2c2868e

…core

jreback added this to the 1.3 milestone Dec 22, 2020

MarcoGorelli added 3 commits December 26, 2020 09:05

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

c6d3233

…core

Dtype -> DtypeObj

1f782fd

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

b315629

…core

jreback requested changes Jan 3, 2021

View reviewed changes

MarcoGorelli added 3 commits January 17, 2021 11:55

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

84207d9

…core

remove redundant parens

3f79955

use set to check for membership

9888383

jreback requested changes Jan 17, 2021

View reviewed changes

MarcoGorelli added 6 commits January 17, 2021 16:22

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

0cd394f

…core

Merge remote-tracking branch 'upstream/master' into refactor-rest-of-…

2ec839e

…core

revert change in generic.py

5125e5f

revert _set_join_index refactoring

d693f58

deduplicate from merge

d4e3bd6

🎨

5559352

MarcoGorelli marked this pull request as draft January 18, 2021 13:17

some reversions

4f14b81

MarcoGorelli mentioned this pull request Jan 18, 2021

CLN refactor maybe-castable #39257

Merged

1 task

revert maybe_castable

c03d876

MarcoGorelli marked this pull request as ready for review January 18, 2021 17:34

jreback approved these changes Jan 20, 2021

View reviewed changes

jreback merged commit fa09c6e into pandas-dev:master Jan 20, 2021

MarcoGorelli deleted the refactor-rest-of-core branch January 20, 2021 17:36

nofarm3 pushed a commit to nofarm3/pandas that referenced this pull request Jan 21, 2021

CLN refactor rest of core (pandas-dev#37586)

de3ca00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN refactor rest of core #37586

CLN refactor rest of core #37586

MarcoGorelli commented Nov 2, 2020

jreback left a comment

jreback Nov 2, 2020

jreback Jan 17, 2021

jbrockmendel Nov 8, 2020

jbrockmendel Nov 8, 2020

jbrockmendel Nov 8, 2020

MarcoGorelli Nov 9, 2020

jbrockmendel Nov 9, 2020

jbrockmendel Nov 8, 2020

jbrockmendel Nov 8, 2020

jbrockmendel Nov 8, 2020

jreback commented Nov 18, 2020

MarcoGorelli commented Nov 18, 2020

jreback commented Nov 24, 2020

MarcoGorelli commented Nov 24, 2020 •

edited

Loading

jreback commented Dec 22, 2020

jreback commented Dec 22, 2020

jreback left a comment

jreback Jan 3, 2021

jreback left a comment

jreback Jan 17, 2021

jreback Jan 17, 2021

jreback commented Jan 20, 2021

		@@ -11106,6 +11091,22 @@ def last_valid_index(self):
		return self._find_valid_index("last")


		def _set_join_index(

CLN refactor rest of core #37586

CLN refactor rest of core #37586

Conversation

MarcoGorelli commented Nov 2, 2020

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

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 commented Nov 18, 2020

MarcoGorelli commented Nov 18, 2020

jreback commented Nov 24, 2020

MarcoGorelli commented Nov 24, 2020 • edited Loading

jreback commented Dec 22, 2020

jreback commented Dec 22, 2020

jreback left a 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

jreback commented Jan 20, 2021

MarcoGorelli commented Nov 24, 2020 •

edited

Loading