Add `particlewise` keyword to `particle_collections.is_category` #2648

jwreep · 2024-04-25T21:42:48Z

Description

Addresses #2595 by adding the requested particlewise keyword to the function particle_collections.is_category.

This changes the default behavior to return a bool rather than a list of bool. If the original functionality is preferred to be the default, the default value of particlewise could be switched to True.

I've added tests that should (hopefully) address all the new cases, by using particlewise as both True and False and adding one new test that should evaluate to True.

I looked at the docs to see if any changes were needed, but the is_category function doesn't appear to have any further description there. Please let me know if I missed something!

Motivation and context

See the description by @namurphy in #2595. It was marked as "needs discussion", which I don't know if this has been resolved.

Related issues

#2595

jwreep · 2024-04-25T22:02:51Z

The ruff error failing is FBT003, from this line
return category_list if particlewise else np.allclose(category_list, True)
which I think is complaining about the call to np.allclose. An alternative formulation, which I think wouldn't trigger the warning, is np.allclose(category_list, np.array([True])) but that's ugly coding and the extra call to np.array will add a small bit of overhead.

There's some relevant discussion in this thread.

Any suggestions?

jwreep · 2024-04-25T22:40:31Z

This caused a lot of other tests to fail because is_category appears to be used pretty commonly in the decorators. I'm guessing to avoid breaking lots of other parts of the code it would be better to switch the default to particlewise = True. Alternatively, we would have to carefully make sure to insert the keyword everywhere the function is called.

Any thoughts?

namurphy · 2024-04-29T22:19:49Z

The ruff error failing is FBT003, from this line return category_list if particlewise else np.allclose(category_list, True) which I think is complaining about the call to np.allclose.

Perhaps we could add a # noqa: FBT003 comment at the end of the corresponding line so that ruff will ignore the rules on those lines?

namurphy · 2024-04-29T22:23:39Z

This caused a lot of other tests to fail because is_category appears to be used pretty commonly in the decorators. I'm guessing to avoid breaking lots of other parts of the code it would be better to switch the default to particlewise = True. Alternatively, we would have to carefully make sure to insert the keyword everywhere the function is called.

In the long term, it would probably be best for Particle.is_category and ParticleList.is_category to return the same type by default. My inclination would be to insert the keyword everywhere the function is called. But, that could possibly be a follow-up PR. 🤔

Thank you for doing this!

codecov · 2024-04-30T00:00:33Z

Codecov Report

Attention: Patch coverage is 95.23810% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 95.20%. Comparing base (820c300) to head (8fe0fd8).

Files	Patch %	Lines
src/plasmapy/particles/particle_collections.py	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2648      +/-   ##
==========================================
- Coverage   95.22%   95.20%   -0.02%     
==========================================
  Files         103      103              
  Lines        9401     9414      +13     
  Branches     2151     2156       +5     
==========================================
+ Hits         8952     8963      +11     
  Misses        274      274              
- Partials      175      177       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jwreep · 2024-04-30T00:06:47Z

Thanks @namurphy!

I've ~~added the suggested noqa~~ replaced np.allclose(category_list, True) with all(category_list) and tried to update the decorator code to fix the issues. It looks like it's solved most of the problems. There's still one outstanding issue with AbstractPhysicalParticle which I'll come back to in a while (looks like it defines its own is_category).

I'm not sure that I understand the outstanding issue with the Documentation, though. Any insight into the issue there?

jwreep · 2024-04-30T02:34:42Z

Sorry for the commits going back and forth. I'm trying to understand the issue with the static type checking.

I've changed ParticleList.is_category to return either bool or list[bool] which is throwing an error in the static type check because any(ParticleList) might not return an iterable. I have it nested under an if-statement that should determine whether it has returned a bool or list[bool], though, so I'm a bit confused why the issue arises.

Edit: still haven't figured this one out. It's raising an error in decorators.verify_allowed_types, which should work just fine (the function won't be called if it's not iterable), but the static type checking still raises an issue with it. Any suggestions would be very helpful. I'm not terribly familiar with the mypy checks.

…ic check?

jwreep · 2024-05-03T03:00:42Z

@namurphy, please review when you can. I finally managed to get the issues sorted.

namurphy

LGTM! 🚀 Thank you for doing this! 🙏🏻

I only have a few quick suggestions, and I think we can get this merged in time for our v2024.5.0 release on Tuesday or Wednesday.

Thank you again!

changelog/2648.feature.rst

namurphy · 2024-05-04T01:08:06Z

docs/particles/particle_class.rst

+As with an individual |Particle| and |CustomParticle|, we can check whether
+all the particles in a list fall within a category using |is_category|.
+
+>>> helium_ions.is_category("ion")
+False
+
+We may also check each particle in the list individually by setting
+the keyword ``particlewise`` to `True`.
+
+>>> helium_ions.is_category("ion", particlewise=True)
+[False, True, True]
+


👍🏻 Thank you for updating this here! I often forget to update the narrative docs until I realize it about six months later... 😅

I only caught this while trying to track down why the docs were failing to build

namurphy · 2024-05-04T01:09:05Z

src/plasmapy/formulary/collisions/helio/collisional_analysis.py

-    if not all(ions.is_category("ion")):
+    if not ions.is_category("ion"):


👍🏻 Always nice to be able to shorten/simplify code!

namurphy · 2024-05-04T01:13:14Z

src/plasmapy/particles/particle_collections.py

+    @overload
    def is_category(
        self,
        *category_tuple,
        require: str | Iterable[str] | None = None,
        any_of: str | Iterable[str] | None = None,
        exclude: str | Iterable[str] | None = None,
-    ) -> list[bool]:
+        particlewise: Literal[True] = ...,
+    ) -> bool: ...
+
+    @overload
+    def is_category(
+        self,
+        *category_tuple,
+        require: str | Iterable[str] | None = None,
+        any_of: str | Iterable[str] | None = None,
+        exclude: str | Iterable[str] | None = None,
+        particlewise: Literal[False],
+    ) -> list[bool]: ...


👍🏻 Nice use of @overload!

So, I did this when I was trying to solve my issues with the static typing. It didn't solve that issue, but from what I gather reading comments on StackExchange and Github, it seems this is a clear way to specify the output when it changes based on a bool input.

That is helpful to know!

If you run into any mypy errors that feel unduly strict, please feel free to bring it up in #2589. We're still in the process of figuring out how strict we want the mypy settings to be, since there are tradeoffs between the benefits of type hints compared to the effort needed to implement them. I suspect that we'll probably need to make mypy somewhat less strict.

I had to use # type: ignore[arg-type] to resolve the issue here:

if ( not self.allow_custom_particles and isinstance(particle, ParticleList) and any(particle.is_category("custom", particlewise=True)) # type: ignore[arg-type] ):

The third condition can only be called if it is indeed a ParticleList, but the mypy check insisted that there would be an issue with possibly return something that is not iterable, which means that the call to any() would fail. It might be too strict for this case? I'll mention it in the other thread.

namurphy · 2024-05-04T01:37:45Z

Thank you for addressing the changes! Everything looks ready, so I'll enable auto-merge.

jwreep · 2024-05-04T01:40:19Z

Thanks for the help!

add particlewise keyword to particle_collections.is_category

8ff6349

jwreep requested a review from a team as a code owner April 25, 2024 21:42

jwreep requested review from namurphy and removed request for a team April 25, 2024 21:42

github-actions bot added the testing label Apr 25, 2024

Jeffrey Reep added 2 commits April 25, 2024 11:44

add changelog

7e2de30

fix precommit

c665e0b

remove brackets in test input

3ed6f26

jwreep and others added 6 commits April 29, 2024 12:35

Merge branch 'PlasmaPy:main' into main

365700f

add noqa

2ac9384

attempt to fix decorators with particlewise=True

c0b34f9

attempt to fix decorators with particlewise=True

d53330e

fix iterable bool issue

5d21d6b

fix is_category error in formulary/collisions/helio/collsional_analysis

d677446

np.allclose -> all(), and remove # noqa

d1967f7

jwreep force-pushed the main branch from 78f07f1 to d1967f7 Compare April 30, 2024 02:30

jwreep and others added 3 commits April 30, 2024 15:27

Merge branch 'PlasmaPy:main' into main

962c683

overload is_category to improve type hints -- does this fix mypy stat…

08b97c8

…ic check?

refactor overload to use Literal[]

fcc75b2

jwreep force-pushed the main branch from be44093 to fcc75b2 Compare May 1, 2024 02:57

Jeffrey Reep and others added 3 commits May 1, 2024 17:00

revert the changes to particles.decorator.verify_allowed_types

79ff73b

Merge branch 'PlasmaPy:main' into main

bfcaa2c

modify mypy checks

0548303

Jeffrey Reep added 2 commits May 2, 2024 15:31

modify docstring

2aeb804

add info to docs about particlewise

925760e

github-actions bot added the docs PlasmaPy Docs at http://docs.plasmapy.org label May 3, 2024

Jeffrey Reep added 3 commits May 2, 2024 16:34

fix changelog + typo

2daceaa

fix docs

b29c854

fix changelog link

783c25d

namurphy approved these changes May 4, 2024

View reviewed changes

namurphy mentioned this pull request May 4, 2024

Make @particle_input apply categorization criteria when creating a ParticleList #2594

Merged

jwreep and others added 2 commits May 3, 2024 15:29

Merge branch 'PlasmaPy:main' into main

f8df663

update changelog

8fe0fd8

github-actions bot added the changes existing API Involves or proposes breaking (backward incompatible) changes label May 4, 2024

namurphy enabled auto-merge (squash) May 4, 2024 01:37

namurphy merged commit ca92246 into PlasmaPy:main May 4, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `particlewise` keyword to `particle_collections.is_category` #2648

Add `particlewise` keyword to `particle_collections.is_category` #2648

jwreep commented Apr 25, 2024

jwreep commented Apr 25, 2024

jwreep commented Apr 25, 2024

namurphy commented Apr 29, 2024

namurphy commented Apr 29, 2024 •

edited

codecov bot commented Apr 30, 2024 •

edited

jwreep commented Apr 30, 2024 •

edited

jwreep commented Apr 30, 2024 •

edited

jwreep commented May 3, 2024

namurphy left a comment

namurphy May 4, 2024

jwreep May 4, 2024

namurphy May 4, 2024

namurphy May 4, 2024

jwreep May 4, 2024

namurphy May 4, 2024

jwreep May 4, 2024

namurphy commented May 4, 2024

jwreep commented May 4, 2024

		if not all(ions.is_category("ion")):
		if not ions.is_category("ion"):

Add particlewise keyword to particle_collections.is_category #2648

Add particlewise keyword to particle_collections.is_category #2648

Conversation

jwreep commented Apr 25, 2024

Description

Motivation and context

Related issues

jwreep commented Apr 25, 2024

jwreep commented Apr 25, 2024

namurphy commented Apr 29, 2024

namurphy commented Apr 29, 2024 • edited

codecov bot commented Apr 30, 2024 • edited

Codecov Report

jwreep commented Apr 30, 2024 • edited

jwreep commented Apr 30, 2024 • edited

jwreep commented May 3, 2024

namurphy left a comment

Choose a reason for hiding this comment

namurphy May 4, 2024

Choose a reason for hiding this comment

jwreep May 4, 2024

Choose a reason for hiding this comment

namurphy May 4, 2024

Choose a reason for hiding this comment

namurphy May 4, 2024

Choose a reason for hiding this comment

jwreep May 4, 2024

Choose a reason for hiding this comment

namurphy May 4, 2024

Choose a reason for hiding this comment

jwreep May 4, 2024

Choose a reason for hiding this comment

namurphy commented May 4, 2024

jwreep commented May 4, 2024

Add `particlewise` keyword to `particle_collections.is_category` #2648

Add `particlewise` keyword to `particle_collections.is_category` #2648

namurphy commented Apr 29, 2024 •

edited

codecov bot commented Apr 30, 2024 •

edited

jwreep commented Apr 30, 2024 •

edited

jwreep commented Apr 30, 2024 •

edited