CLN: annotation in reshape.merge #29490

jbrockmendel · 2019-11-08T17:06:10Z

This will need multiple passes, trying to keep a moderately-sized diff.

simonjayhawkins

Thanks @jbrockmendel. minor comments otherwise lgtm.

pandas/core/reshape/merge.py

simonjayhawkins · 2019-11-08T22:23:40Z

pandas/core/reshape/merge.py

-    rcodes, lcodes, shape = map(list, zip(*map(fkeys, index.levels, join_keys)))
+    mapped = [fkeys(index.levels[n], join_keys[n]) for n in range(len(index.levels))]
+    zipped = zip(*mapped)
+    rcodes, lcodes, shape = [list(x) for x in zipped]


was this cleaning or silencing mypy?

this was necessary to silence mypy. i also dont like the pattern that was used before (and am not wild about the zip still here, open to suggestions)

yep. fine with this. just curious.

OK with splitting this out but should probably keep mapped as a generator; coercing to list may have some overhead

pandas/core/reshape/merge.py

simonjayhawkins · 2019-11-08T22:25:19Z

pandas/core/reshape/merge.py

@@ -577,7 +581,7 @@ def __init__(
        self.indicator = indicator

        if isinstance(self.indicator, str):
-            self.indicator_name = self.indicator
+            self.indicator_name = self.indicator  # type: Optional[str]


use py3.6 syntax :)

my preference would be to declare outside the if else.

WillAyd · 2019-11-10T00:34:46Z

pandas/core/reshape/merge.py

-        left,
-        right,
-        how="inner",
+        left: FrameOrSeries,


Haven't debugged to verify but we can mix and match objects here right? So something as follows:

>>> df = pd.DataFrame([[1]]) >>> ser = pd.Series([1], name="a") >>> pd.merge(df, ser, left_index=True, right_index=True) 0 a 0 1 1

If that's the case then these actually should be Union[DataFrame, Series] instead of FrameOrSeries. Pretty tricky

Correct, left and right can each be either DataFrame or Series, i.e. 4 valid combinations. Does FrameOrSeries assume you're not mix-and-matching? will update

WillAyd · 2019-11-10T00:36:02Z

pandas/core/reshape/merge.py

-    rcodes, lcodes, shape = map(list, zip(*map(fkeys, index.levels, join_keys)))
+    mapped = [fkeys(index.levels[n], join_keys[n]) for n in range(len(index.levels))]
+    zipped = zip(*mapped)
+    rcodes, lcodes, shape = [list(x) for x in zipped]


OK with splitting this out but should probably keep mapped as a generator; coercing to list may have some overhead

…n-merge

simonjayhawkins

@jbrockmendel lgtm pending @WillAyd comment #29490 (comment)

simonjayhawkins · 2019-11-10T17:10:48Z

pandas/core/reshape/merge.py

-        left: FrameOrSeries,
-        right: FrameOrSeries,
+        left: "Union[Series, DataFrame]",
+        right: "Union[Series, DataFrame]",


needs quotes?

"Union[Series, DataFrame]" -> Union["Series", "DataFrame"]

I think is clearer. not sure if we have a preferred style here. @WillAyd ?

I agree that having the quotes inside the union would be nicer, but doing so gave flake8 complaints about Series being unused.

adding # noqa: F401 to the imports inside the TYPE_CHECKING block I think is acceptable and not uncommon in this case.

jbrockmendel · 2019-11-11T15:39:38Z

@WillAyd has your comment #29490 (comment) been addressed?

WillAyd · 2019-11-11T16:04:47Z

pandas/core/reshape/merge.py

    """

    _merge_type = "merge"

+    indicator_name: Optional[str]
+    left: "DataFrame"


Is there a reason for adding these definitions at the class level? Would prefer not to do that

there was a request to use py36 style annotations

Can these be done in the init? These are instance variables shouldn't need to expose as class variables now for annotations

i guess so, sure. is this the wrong use case for putting these a the class level? or is it a preference/policy thing?

Can these be done in the init? These are instance variables shouldn't need to expose as class variables now for annotations

since these are type declarations and not variable initialisations, I think that these are a noop at runtime and therefore not exposed as class variables.

>>> class foo(): ... bar: str ... >>> foo().bar Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'foo' object has no attribute 'bar' >>> foo.bar Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: type object 'foo' has no attribute 'bar'

Is there consensus on the desired usage?

If it can be done in the init I still think better. Should keep things localized and not push to higher namespaces unless really necessary

…n-merge

jbrockmendel · 2019-11-12T16:34:00Z

@simonjayhawkins Will has given the OK, back to you

jreback

minor comment

jreback · 2019-11-12T23:41:48Z

pandas/core/reshape/merge.py

-        self.right = self.orig_right = right
+        _left = _validate_operand(left)
+        _right = _validate_operand(right)
+        self.left = self.orig_left = _validate_operand(_left)  # type: "DataFrame"


yeah i think can update to 36 syntax here

actually if you can do in a followon is fine.

sounds good

and remove the duplicated _validate_operand that crept in in the final commit.

CLN: annotation in reshape.merge

8384e5f

simonjayhawkins requested changes Nov 8, 2019

View reviewed changes

simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Nov 8, 2019

simonjayhawkins added this to the 1.0 milestone Nov 8, 2019

jbrockmendel added 2 commits November 8, 2019 14:48

update per comments

bc15a94

restore assignment

6b3f6cb

WillAyd requested changes Nov 10, 2019

View reviewed changes

jbrockmendel added 2 commits November 10, 2019 08:52

Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…

a58d81e

…n-merge

revert to generator

de5fe3b

simonjayhawkins approved these changes Nov 10, 2019

View reviewed changes

update per comment

731cea7

simonjayhawkins reviewed Nov 10, 2019

View reviewed changes

suggested typing edit

ed3c56c

WillAyd requested changes Nov 11, 2019

View reviewed changes

jbrockmendel added 2 commits November 11, 2019 08:27

Merge branch 'master' of https://github.com/pandas-dev/pandas into cl…

c33a91b

…n-merge

update per requests

b34a9ac

WillAyd approved these changes Nov 11, 2019

View reviewed changes

jreback requested changes Nov 12, 2019

View reviewed changes

jreback approved these changes Nov 12, 2019

View reviewed changes

jreback merged commit 808f482 into pandas-dev:master Nov 12, 2019

jbrockmendel deleted the cln-merge branch November 13, 2019 00:09

jbrockmendel added a commit to jbrockmendel/pandas that referenced this pull request Nov 13, 2019

CLN: requested follow-up to pandas-dev#29490

fb87982

Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019

CLN: annotation in reshape.merge (pandas-dev#29490)

6dde106

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

CLN: annotation in reshape.merge (pandas-dev#29490)

d2e6dc2

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

CLN: annotation in reshape.merge (pandas-dev#29490)

8b61bf5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: annotation in reshape.merge #29490

CLN: annotation in reshape.merge #29490

jbrockmendel commented Nov 8, 2019

simonjayhawkins left a comment

simonjayhawkins Nov 8, 2019

jbrockmendel Nov 8, 2019

simonjayhawkins Nov 8, 2019

WillAyd Nov 10, 2019

simonjayhawkins Nov 8, 2019

WillAyd Nov 10, 2019

jbrockmendel Nov 10, 2019

WillAyd Nov 10, 2019

simonjayhawkins left a comment

simonjayhawkins Nov 10, 2019

simonjayhawkins Nov 10, 2019

jbrockmendel Nov 10, 2019

simonjayhawkins Nov 10, 2019

jbrockmendel Nov 10, 2019

jbrockmendel commented Nov 11, 2019

WillAyd Nov 11, 2019

jbrockmendel Nov 11, 2019

WillAyd Nov 11, 2019

jbrockmendel Nov 11, 2019

simonjayhawkins Nov 11, 2019 •

edited

Loading

jbrockmendel Nov 11, 2019

WillAyd Nov 11, 2019

simonjayhawkins Nov 11, 2019

jbrockmendel Nov 11, 2019

jbrockmendel commented Nov 12, 2019

jreback left a comment

jreback Nov 12, 2019

jreback Nov 12, 2019

jbrockmendel Nov 13, 2019

simonjayhawkins Nov 13, 2019

CLN: annotation in reshape.merge #29490

CLN: annotation in reshape.merge #29490

Conversation

jbrockmendel commented Nov 8, 2019

simonjayhawkins 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

simonjayhawkins 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

jbrockmendel commented Nov 11, 2019

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

simonjayhawkins Nov 11, 2019 • edited Loading

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 Nov 12, 2019

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

simonjayhawkins Nov 11, 2019 •

edited

Loading