STYLE: turn off PLW2901 (but keep some changes) #52262

ErdiTk · 2023-03-28T20:12:33Z

Continuation of work started on closed issue #51708.
Didn't find a new issue to link this PR, may I ask you @MarcoGorelli if it is fine to link it to the closed one or shall I wait for a new issue to be opened?

MarcoGorelli

thanks

can you update the pyproject.toml file accordingly please?

ErdiTk · 2023-03-28T20:51:07Z

thanks

can you update the pyproject.toml file accordingly please?

I will update it when I finish all the files under pandas/core/*. Give me few days for it, there are quite some to change :)

MarcoGorelli · 2023-03-28T21:00:37Z

it's fine (and preferable) split it up into smaller chunks

we can split up the pandas/core/* ignore into multiple ones

MarcoGorelli

looks like the "
Code Checks / Docstring validation, typing, and other manual pre-commit hooks (pull_request) " is failing - code you give me a ping when that's one's fixed please?

ErdiTk · 2023-03-31T09:32:02Z

looks like the "
Code Checks / Docstring validation, typing, and other manual pre-commit hooks (pull_request) " is failing - code you give me a ping when that's one's fixed please?

Sure, I think I know which changes might have impacted it. I will ping you around Tuesday, unfortunately I won't make it earlier.

MarcoGorelli · 2023-03-31T11:12:08Z

sure, no hurry, thanks!

…to aggfunc if just returned.

ErdiTk · 2023-04-04T21:03:49Z

pandas/core/apply.py

+            mangled_aggfuncs.append(aggfunc)
+            continue
+
+        partial_aggfunc = aggfunc


@MarcoGorelli by adding this line the error should be solved. At least locally the mypy error is gone. Not sure why but by simply creating partial_aggfunc from the call to partial(aggfunc) in the next line it removes the __name__ attribute. But if I firstly assign it as equal to aggfunc as I am doing here it works fine. Are you aware of any similar behavior?

The reason this fixes it is because it turns off the type checker - agg_func is of type Any (which turns off the type checker), and so partial_aggfunc will also be of type Any, and so mypy will pass (because it doesn't check anything about this variable)

You can verify this by adding reveal_type:

partial_aggfunc = aggfunc reveal_type(partial_aggfunc) partial_aggfunc = partial(aggfunc) reveal_type(partial_aggfunc)

which will show

pandas/core/apply.py:1438: note: Revealed type is "Any" pandas/core/apply.py:1440: note: Revealed type is "Any"

If we remove this line, then partial_aggfunc will be inferred to be of type Partial[Any]

I'd suggest just ignoring the type checker here, like

partial_aggfunc = partial(aggfunc) # "partial[Any]" has no attribute "__name__" partial_aggfunc.__name__ = f"<lambda_{idx}>" # type: ignore[attr-defined]

Oh thank you a lot for the suggestion! I will implement it like this. Sorry but I am still learning how to properly handle those issues.

MarcoGorelli

thanks for your PR

I've left a couple of comments, there should be enough to work on here

MarcoGorelli · 2023-04-05T13:57:31Z

pandas/core/apply.py

+            mangled_aggfuncs.append(aggfunc)
+            continue
+
+        partial_aggfunc = aggfunc


The reason this fixes it is because it turns off the type checker - agg_func is of type Any (which turns off the type checker), and so partial_aggfunc will also be of type Any, and so mypy will pass (because it doesn't check anything about this variable)

You can verify this by adding reveal_type:

partial_aggfunc = aggfunc reveal_type(partial_aggfunc) partial_aggfunc = partial(aggfunc) reveal_type(partial_aggfunc)

which will show

pandas/core/apply.py:1438: note: Revealed type is "Any" pandas/core/apply.py:1440: note: Revealed type is "Any"

If we remove this line, then partial_aggfunc will be inferred to be of type Partial[Any]

I'd suggest just ignoring the type checker here, like

partial_aggfunc = partial(aggfunc) # "partial[Any]" has no attribute "__name__" partial_aggfunc.__name__ = f"<lambda_{idx}>" # type: ignore[attr-defined]

MarcoGorelli · 2023-04-05T14:07:23Z

pandas/core/apply.py

+        partial_aggfunc = partial(aggfunc)
+        partial_aggfunc.__name__ = f"<lambda_{idx}>"


also, I don't think the logic is right here, please try to keep the logic the same as the original

The original one was:

i = 0 for aggfunc in aggfuncs: if com.get_callable_name(aggfunc) == "<lambda>": aggfunc = partial(aggfunc) aggfunc.__name__ = f"<lambda_{i}>" i += 1 mangled_aggfuncs.append(aggfunc)

I removed the variable i, replacing it by idx generated by the call to enumerate. Then I inverted the if clause, making it a if not. In this way I can insert inside the condition the single line:
mangled_aggfuncs.append(aggfunc)
and then continue the loop. So I moved what was inside the if outside. Sorry I understand it's not super clear, but I thought would look nicer to have a lighter content inside the if statement. If you want I can revert the logic.

MarcoGorelli · 2023-04-05T14:07:45Z

pandas/core/arrays/period.py

-            y, m = parsing.quarter_to_myear(y, q, freqstr)
+            _, m = parsing.quarter_to_myear(y, q, freqstr)
            val = libperiod.period_ordinal(y, m, 1, 1, 1, 1, 0, 0, base)


this doesn't look right

My bad, indeed the variable y can be changed inside the function quarter_to_myear. Updated! Thanks for the heads up!

MarcoGorelli · 2023-04-11T13:57:06Z

pandas/core/apply.py

-    for aggfunc in aggfuncs:
-        if com.get_callable_name(aggfunc) == "<lambda>":
-            aggfunc = partial(aggfunc)
-            aggfunc.__name__ = f"<lambda_{i}>"
-            i += 1
-        mangled_aggfuncs.append(aggfunc)
+    for idx, aggfunc in enumerate(aggfuncs):
+        if not com.get_callable_name(aggfunc) == "<lambda>":
+            mangled_aggfuncs.append(aggfunc)
+            continue
+
+        partial_aggfunc = partial(aggfunc)
+        # "partial[Any]" has no attribute "__name__"
+        partial_aggfunc.__name__ = f"<lambda_{idx}>"  # type: ignore[attr-defined]
+        mangled_aggfuncs.append(partial_aggfunc)


this isn't the same logic - before, i was only incremented if com.get_callable_name(aggfunc) == "<lambda>", whereas now you're incrementing idx on every iteration

Oh correct indeed, thanks I will revert to the original logic. Shall I add also a test to catch this?

MarcoGorelli · 2023-04-11T13:58:46Z

pandas/core/arrays/period.py

@@ -1105,8 +1105,8 @@ def _range_from_fields(
        freqstr = freq.freqstr
        year, quarter = _make_field_arrays(year, quarter)
        for y, q in zip(year, quarter):
-            y, m = parsing.quarter_to_myear(y, q, freqstr)
-            val = libperiod.period_ordinal(y, m, 1, 1, 1, 1, 0, 0, base)
+            y_parsed, m = parsing.quarter_to_myear(y, q, freqstr)


maybe calendar_year, calendar_month = parsing.quarter_to_myear(y, q, freqstr)?

MarcoGorelli

thanks for working on this

I'm going to go back to my previous comment #51797 (review) , I'm not sure we want to enable this in CI

some of these changes are nice

shall we keep the

where[idx] = _validate_where(w)

change (which is quite nice), revert the rest, and move PLW2901 to the 'intentionally turned off' section of pyproject.toml?

pandas/pyproject.toml

Line 316 in af55880

# intentionally turned off

ErdiTk · 2023-04-11T20:44:07Z

Ok, I reverted most of the changes except on few files.
Under # intentionally turned off I added "redefined-loop-name", but I saw the errors still appearing when running pre-commit run ruff --all-files, so I kept "PLW2901" also under the to ignore section of ruff. Please let me know if this is the correct way. Also let me know if the few changes listed are fine.

MarcoGorelli · 2023-04-12T14:59:58Z

pandas/core/apply.py

@@ -1423,10 +1423,10 @@ def relabel_result(
        # mean  1.5
        # mean  1.5
        if reorder_mask:
-            fun = [


not sure we can do this, fun is still used below

MarcoGorelli

looks fine, just made some minor fixup, thanks @ErdiTk

ErdiTk added 2 commits March 28, 2023 21:26

Removed unnecessary code

61fad93

Enabled PLW2901 on initial files under pandas/core/*

fc7a8d8

ErdiTk force-pushed the gradually_enable_plw2901 branch from 6e93f07 to fc7a8d8 Compare March 28, 2023 20:25

MarcoGorelli self-requested a review March 28, 2023 20:43

MarcoGorelli suggested changes Mar 28, 2023

View reviewed changes

ErdiTk added 2 commits March 29, 2023 18:47

Divided pandas/core/* in pyproject.toml. Included some more fixes.

7bbbdb5

Merge branch 'main' into gradually_enable_plw2901

fb81fc0

mroeschke added the Code Style Code style, linting, code_checks label Mar 29, 2023

MarcoGorelli suggested changes Mar 31, 2023

View reviewed changes

ErdiTk added 2 commits April 4, 2023 22:56

Not sure why but the call to partial removes the .__name__ attribute …

33b6b7f

…to aggfunc if just returned.

Merge branch 'main' into gradually_enable_plw2901

73218fa

ErdiTk commented Apr 4, 2023

View reviewed changes

MarcoGorelli suggested changes Apr 5, 2023

View reviewed changes

ErdiTk added 3 commits April 6, 2023 21:50

Ignored type checker and corrected possible induced bug in period.py

d561fbe

Merge branch 'main' into gradually_enable_plw2901

f30d802

Forgot to add the not in previous commit

7807b06

MarcoGorelli suggested changes Apr 11, 2023

View reviewed changes

MarcoGorelli reviewed Apr 11, 2023

View reviewed changes

MarcoGorelli suggested changes Apr 11, 2023

View reviewed changes

ErdiTk added 2 commits April 11, 2023 22:31

Reverted changes and updated pyproject.toml

56ed5aa

Merge branch 'main' into gradually_enable_plw2901

a7749e9

turn off redefined-loop-variable, minimise diff

c0f4be5

MarcoGorelli added this to the 2.1 milestone Apr 12, 2023

MarcoGorelli changed the title ~~Gradually enable PLW2901 on some files~~ STYLE: turn off PLW2901 (but keep some changes) Apr 12, 2023

revert fun_name change

5d3ca8b

MarcoGorelli reviewed Apr 12, 2023

View reviewed changes

MarcoGorelli approved these changes Apr 12, 2023

View reviewed changes

MarcoGorelli merged commit 8d4cab5 into pandas-dev:main Apr 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STYLE: turn off PLW2901 (but keep some changes) #52262

STYLE: turn off PLW2901 (but keep some changes) #52262

ErdiTk commented Mar 28, 2023

MarcoGorelli left a comment

ErdiTk commented Mar 28, 2023

MarcoGorelli commented Mar 28, 2023

MarcoGorelli left a comment

ErdiTk commented Mar 31, 2023

MarcoGorelli commented Mar 31, 2023

ErdiTk Apr 4, 2023 •

edited

Loading

MarcoGorelli Apr 5, 2023

ErdiTk Apr 6, 2023

MarcoGorelli left a comment

MarcoGorelli Apr 5, 2023

MarcoGorelli Apr 5, 2023

ErdiTk Apr 6, 2023

MarcoGorelli Apr 5, 2023

ErdiTk Apr 6, 2023

MarcoGorelli Apr 11, 2023

ErdiTk Apr 11, 2023

MarcoGorelli Apr 11, 2023

MarcoGorelli left a comment

ErdiTk commented Apr 11, 2023

MarcoGorelli Apr 12, 2023

MarcoGorelli left a comment

		partial_aggfunc = partial(aggfunc)
		partial_aggfunc.__name__ = f"<lambda_{idx}>"

STYLE: turn off PLW2901 (but keep some changes) #52262

STYLE: turn off PLW2901 (but keep some changes) #52262

Conversation

ErdiTk commented Mar 28, 2023

MarcoGorelli left a comment

Choose a reason for hiding this comment

ErdiTk commented Mar 28, 2023

MarcoGorelli commented Mar 28, 2023

MarcoGorelli left a comment

Choose a reason for hiding this comment

ErdiTk commented Mar 31, 2023

MarcoGorelli commented Mar 31, 2023

ErdiTk Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli 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

MarcoGorelli left a comment

Choose a reason for hiding this comment

ErdiTk commented Apr 11, 2023

Choose a reason for hiding this comment

MarcoGorelli left a comment

Choose a reason for hiding this comment

ErdiTk Apr 4, 2023 •

edited

Loading