fix: ruff c408 in astropy/units by paverett · Pull Request #15664 · astropy/astropy

paverett · 2023-12-01T03:54:42Z

Description

This pull request is to address violations to Ruff's rule C408, which is listed explicitly in issue #14818. Only the units sub module is addressed in this PR.

Solves part of #14818.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2023-12-01T03:55:14Z

eerovaher · 2023-12-01T11:46:49Z

Hi, @paverett

One of the purposes of #14818 is to suggest very simple tasks to new contributors so that they can familiarize themselves with the basic workflow and tools that we use to develop astropy. Based on your pull requests I think you should be capable of addressing more challenging rules now. I encourage you to work on PT012, but if you would like to address some other rule not explicitly listed in #14818 then feel free to do so.

paverett · 2023-12-01T15:41:39Z

Hi, @paverett

One of the purposes of #14818 is to suggest very simple tasks to new contributors so that they can familiarize themselves with the basic workflow and tools that we use to develop astropy. Based on your pull requests I think you should be capable of addressing more challenging rules now. I encourage you to work on PT012, but if you would like to address some other rule not explicitly listed in #14818 then feel free to do so.

I understand, sorry about that @eerovaher. Yeah I can take a look at PT012! I looked and didn't find an issue regarding PT012. Should I create one or did I miss one that has already been created?

mhvk

Looking at this, I actually rather dislike the changes, in particular for the usage of dict - when used to set kwargs, I find it rather nice that it looks like passing on arguments to a function call.

So, I'd suggest to keep ignoring this particular ruff rule, and only change the cases where it is an empty tuple, list, or dict (where not parametrized). I marked the cases I think are fine to change.

mhvk · 2023-12-01T16:17:10Z

        neginf = x._to_own_unit(neginf)
    return (
        (x.view(np.ndarray),),
-        dict(copy=True, nan=nan, posinf=posinf, neginf=neginf),


I do not like this: Here, kwargs is set and I find it much clearer if it looks like arguments to a function.

This is a helper function for a numpy operation so speed is very important.
Given that, this is a good improvement.

The speed advantage is completely negligible compared to the overhead of even this function itself. I made a conscious decision to write it as it is here, and I don't think this change is good.

But if you want to speed it up further and keep the intent clear, I'm fine with changing the code to

return (x.view(np.ndarray), copy, nan, posinf, neginf), {}, x.unit, None

(I guess that would continue the now common practice to rewrite code just to avoid black formatting...)

I wonder what the speed of constructing a NamedTuple is.

mhvk · 2023-12-01T16:17:22Z

-            usemask=usemask,
-            asrecarray=asrecarray,
-        ),
+        {


Same, please exclude this one.

This is a helper function for a numpy operation so speed is very important.
Given that, this is a good improvement.

As above. Can "solve" the same way too if we really want to follow ruff gospel.

mhvk · 2023-12-01T16:23:28Z

        self.data = equiv_list
        self.name = [name]
-        self.kwargs = [kwargs] if kwargs is not None else [dict()]
+        self.kwargs = [kwargs] if kwargs is not None else [{}]


This one is fine.

mhvk · 2023-12-01T16:23:37Z


        except (TypeError, ValueError, AttributeError) as e:
-            out_normalized = kwargs.get("out", tuple())
+            out_normalized = kwargs.get("out", ())


Definitely better

mhvk · 2023-12-01T16:24:22Z

    assert e1.name == ["pixel_scale", "temperature_energy"]
    assert isinstance(e1.kwargs, list)
-    assert e1.kwargs == [{"pixscale": 10 * u.arcsec / u.pix}, dict()]
+    assert e1.kwargs == [{"pixscale": 10 * u.arcsec / u.pix}, {}]


mhvk · 2023-12-01T16:24:34Z



-@pytest.mark.parametrize("multiplicand", [list(), 42, 0, -1])
+@pytest.mark.parametrize("multiplicand", [[], 42, 0, -1])


Definitely better

nstarman · 2023-12-01T21:45:23Z

@mhvk, there are 2 good reasons why using the built-in dict constructor {} is better than the dict callable:

Speed. It's like 2x faster to use the dict constructor. Applying Ruff C408 is an easy speedup wherever it's fixed.

Details

Safety. Python does not protect dict as a symbol so it can be re-assigned. Because of how Python scopes namespaces the following thing can happpen:

Details

In sum, {} is 100% equivalent but 2x faster and safer.

nstarman

As I note in a comment this PR changes the dict callable for the built-in dict constructor, which is 2x faster and not susceptible to namespace trickery. Thanks @paverett for contributing. IMO this is a good PR to merge.

nstarman · 2023-12-01T21:48:57Z

-            usemask=usemask,
-            asrecarray=asrecarray,
-        ),
+        {


This is a helper function for a numpy operation so speed is very important.
Given that, this is a good improvement.

nstarman · 2023-12-01T21:49:09Z



-@pytest.mark.parametrize("multiplicand", [list(), 42, 0, -1])
+@pytest.mark.parametrize("multiplicand", [[], 42, 0, -1])


mhvk · 2023-12-01T22:12:45Z

    @pytest.mark.parametrize(
        "kwargs",
        (
-            dict(return_index=True, return_inverse=True),


All these test cases are very specifically written this way to make them look like arguments to functions. I really see no reason to decrease readability because of some ruff "rule".

eerovaher · 2023-12-06T14:47:26Z

When I added C408 to the list of suggested rules I didn't expect it to be controversial, and it has been applied in quite a few sub-packages without provoking discussion. But if the sub-package maintainers don't want the rule to be enforced in all cases then for now it's best to just apply it where they are happy with it.

@paverett asked:

I looked and didn't find an issue regarding PT012. Should I create one or did I miss one that has already been created?

I don't see any value in opening issues for that rule. You can open pull requests without having to refer to any specific issue.

paverett · 2024-01-19T02:07:49Z

@nstarman @mhvk I am unsure how i should continue with this PR, should I continue to implement the changes requested in this PR? Or should I close it? It does not seem like a change that is wanted.

eerovaher · 2024-01-19T13:49:34Z

@mhvk, would you be interested in turning on Ruff's allow-dict-calls-with-keyword-arguments setting?

mhvk · 2024-01-19T15:46:47Z

@mhvk, would you be interested in turning on Ruff's allow-dict-calls-with-keyword-arguments setting?

Yes, that would make sense, so that means for dict C408 just enforces empty dict() should be {}, right? (and still does empty tuples and lists, etc.

nstarman · 2024-01-19T15:57:46Z

I'm fine with that. We can return to the question of dicts later / never. Some refactoring might make the question moot.
It would be nice if the allow-dict-calls-with-keyword-arguments could be done in a file-specific way, e.g. to ignore the test files & select unit files, but that's not a show-stopper.

eerovaher · 2024-01-19T16:50:01Z

@paverett, when #15915 is merged then you can address the remaining violations in units.

mhvk

Sorry to be a bit slow - this one is now all OK. Thanks!

mhvk · 2024-01-30T15:48:43Z

Failure unrelated, so merging. Thanks again!

paverett requested review from WilliamJamieson and nstarman as code owners December 1, 2023 03:54

github-actions Bot added the units label Dec 1, 2023

pllim added this to the v6.1.0 milestone Dec 1, 2023

pllim added Refactoring no-changelog-entry-needed dev-automation labels Dec 1, 2023

neutrinoceros approved these changes Dec 1, 2023

View reviewed changes

mhvk requested changes Dec 1, 2023

View reviewed changes

mhvk mentioned this pull request Dec 1, 2023

Ensure compliance with more Ruff rules #14818

Closed

3 of 6 tasks

nstarman approved these changes Dec 1, 2023

View reviewed changes

mhvk reviewed Dec 1, 2023

View reviewed changes

eerovaher mentioned this pull request Jan 19, 2024

Enable Ruff allow-dict-calls-with-keyword-arguments setting #15915

Merged

1 task done

paverett added 3 commits January 25, 2024 19:08

fix: fix ruff c408 in astropy/units

a9f0949

fix: misspelled key, list construction error

3f30ace

fix: remove dict calls with arguments

f390995

paverett force-pushed the fix/ruff_c408_units branch from 8b7c3af to f390995 Compare January 26, 2024 02:33

mhvk approved these changes Jan 30, 2024

View reviewed changes

mhvk merged commit 95c510a into astropy:main Jan 30, 2024



		@pytest.mark.parametrize("multiplicand", [list(), 42, 0, -1])
		@pytest.mark.parametrize("multiplicand", [[], 42, 0, -1])

Conversation

paverett commented Dec 1, 2023

Description

Uh oh!

github-actions Bot commented Dec 1, 2023

Uh oh!

eerovaher commented Dec 1, 2023

Uh oh!

paverett commented Dec 1, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman commented Dec 1, 2023

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Dec 6, 2023

Uh oh!

paverett commented Jan 19, 2024

Uh oh!

eerovaher commented Jan 19, 2024

Uh oh!

mhvk commented Jan 19, 2024

Uh oh!

nstarman commented Jan 19, 2024

Uh oh!

eerovaher commented Jan 19, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jan 30, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mhvk Dec 1, 2023 •

edited

Loading