Add separate constructor for `CustomParticle` and improve |ParticleList| compatibility with |Quantity| objects #1874

namurphy · 2023-01-06T21:02:00Z

This PR makes the following changes:

Changed the signature of CustomParticle to CustomParticle(*quantities, ...), where quantities can include Quantity objects of physical type mass or charge, or (as its last item) the symbol. See Add mechanism to instantiate CustomParticle based off of positional arguments #1873 for some motivation and follow-up discussion. This is a follow-up attempt to do more or less the same thing as Provide more flexibility in CustomParticle arguments #1364, which didn't get merged. Closes Add mechanism to instantiate CustomParticle based off of positional arguments #1873.
Fixed a new edge case in the particle factory function by assuming that a string passed as the first positional argument to it is meant for Particle and not as a symbol for CustomParticle.
Made ParticleList.__init__ accept an iterable containing Quantity objects. Closes Make ParticleList compatible with Quantity objects with physical type of mass or electrical charge #1872.
Made ParticleList methods accept Quantity objects of mass or electrical charge.

The changes to CustomParticle made the changes to ParticleList pretty straightforward to implement, so I addressed them here.

This PR should make it easier to address #1869 and #1871.

github-actions · 2023-01-06T21:02:32Z

namurphy · 2023-01-06T21:04:34Z

This PR is still in its early stages, and I'm still thinking about the implementation details.

I'm going to try to reduce breaking changes, but it's likely this would make symbol keyword-only which would be a breaking changes. I'm also wondering if the mass and charge parameters should be deprecated.

αβtize!

codecov · 2023-01-08T20:02:04Z

Codecov Report

Base: 98.31% // Head: 98.31% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (307ee75) compared to base (83375f8).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1874      +/-   ##
==========================================
- Coverage   98.31%   98.31%   -0.01%     
==========================================
  Files          95       95              
  Lines        8383     8424      +41     
==========================================
+ Hits         8242     8282      +40     
- Misses        141      142       +1

Impacted Files	Coverage Δ
plasmapy/particles/_factory.py	`100.00% <100.00%> (ø)`
plasmapy/particles/particle_class.py	`98.56% <100.00%> (+0.04%)`	⬆️
plasmapy/particles/particle_collections.py	`100.00% <100.00%> (ø)`
plasmapy/utils/_units_helpers.py	`96.00% <100.00%> (-4.00%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

StanczakDominik

Could use some cleaning up imo!

StanczakDominik · 2023-01-09T14:33:19Z

plasmapy/particles/tests/test_particle_collections.py

+            5 * u.kg,
+            5 * u.C,


So these two will create, respectively, a charge-less 5kg chonkyboi of a particle and a massless one with a massive 5 C charge, both custom ones, right?

StanczakDominik · 2023-01-09T14:40:17Z

plasmapy/particles/_factory.py

    for particle_type in (Particle, CustomParticle, ParticleList):
-        with contextlib.suppress(TypeError, InvalidParticleError):
+        with contextlib.suppress(TypeError, InvalidParticleError, ValueError):
+
+            # Do not interpret a string as the first positional argument
+            # as the symbol for a CustomParticle or something to be
+            # iterated over for a ParticleList.
+            if particle_type is not Particle and args and isinstance(args[0], str):
+                continue
+
            return particle_type(*args, **kwargs)


Might be myself being tired but this whole block seems difficult to reason about! This needs a refactor:

let's enter suppress context once instead of within the for loop

the case for Particle should probably be separate

Yeah, this block is a bit hard to wrap one's mind around.

The reason why contextlib.suppress is used within the for block is that it tries to create a Particle, CustomParticle, and ParticleList sequentially. There's a chance for one of these exceptions to be raised each time. If the suppress context were outside the for loop, then it would exit the context environment upon the first exception...I think. If it failed to create a Particle, then it wouldn't try to create a CustomParticle or ParticleList afterwards.

I'll keep thinking about how to clean this up and reduce the cognitive complexity.

StanczakDominik · 2023-01-09T14:41:41Z

plasmapy/particles/particle_class.py

+    *quantities : tuple of ~astropy.units.Quantity or `str`
+        The mass, electrical charge, and/or symbol of the custom
+        particle, provided as positional arguments. The mass and charge
+        may be provided in either order, but the symbol may only be the
+        final positional argument.

-    charge : ~astropy.units.Quantity or ~numbers.Real, optional
-        The electric charge of the custom particle.  If provided as a
-        `~astropy.units.Quantity`, then it must be in units of electric
-        charge. Defaults to |nan| C.
+    charge : ~astropy.units.Quantity or ~numbers.Real, |keyword-only|, optional
+        The electrical charge of the custom particle, if not provided in
+        ``quantities`` as a positional argument.

-    Z : ~numbers.Real, optional, |keyword-only|
-        The :term:`charge number`, which is equal to the ratio of the
-        charge to the elementary charge.
+    mass : ~astropy.units.Quantity, |keyword-only|, |keyword-only|, optional
+        The mass of the custom particle, if not provided in
+        ``quantities`` as a positional argument.
+
+    symbol : str, |keyword-only|, |keyword-only|, optional
+        The symbol to be assigned to the custom particle, if not
+        provided as the final positional argument.

-    symbol : str, optional
-        The symbol to be assigned to the custom particle.
+    Z : ~numbers.Real, optional, |keyword-only|
+        The |charge number|, which is equal to the ratio of the charge
+        to the elementary charge.


I kind of dislike how this whole change actually made the user-facing complexity, as measured by the length of docstring necessary, higher! Nothing to change here, probably, just voicing my concern.

Agreed — the downside of this change is the increased user-facing complexity. My original plan was to deprecate the mass and charge keyword arguments, which I haven't done but am still considering. The extra complexity comes from attempting to preserve backward compatibility where mass and charge can be used as keyword arguments too.

StanczakDominik · 2023-01-09T14:44:23Z

plasmapy/particles/particle_class.py

+            if isinstance(quantities[-1], str) and symbol is None:
+                symbol = quantities[-1]
+                quantities = list(quantities[:-1])
+
+            try:
+                physical_type_dict = _get_physical_type_dict(
+                    quantities,
+                    only_quantities=True,
+                    strict=True,
+                    allowed_physical_types={
+                        u.physical.mass,
+                        u.physical.electrical_charge,
+                    },
+                )
+            except (TypeError, ValueError) as exc:
+                raise InvalidParticleError(
+                    "Unable to create a CustomParticle based off of the "
+                    "following positional arguments, which should be a"
+                    f"Quantity representing electrical charge and/or a"
+                    f"Quantity representing mass: {quantities}."
+                ) from exc
+
+            if u.physical.electrical_charge in physical_type_dict:
+                if charge is not None:
+                    raise TypeError(
+                        "Cannot provide charge as both a positional and keyword "
+                        "argument."
+                    )
+                charge = physical_type_dict[u.physical.electrical_charge]
+
+            if u.physical.mass in physical_type_dict:
+                if mass is not None:
+                    raise TypeError(
+                        "Cannot provide mass as both a positional and keyword argument."
+                    )
+                mass = physical_type_dict[u.physical.mass]
+


Since this whole block is pretty separate from other functionality, how about moving it to a separate semi-private constructor like this...

@classmethod def _from_positional(cls, args): ... # this whole block

and then calling that from here if quantities is non-empty?

Agreed. This would be cleaner by extracting a bunch of this into its own method.

namurphy · 2023-01-09T21:51:20Z

My main question at this point is: would it be better to deprecate the mass and charge keyword arguments, and make the mass and charge positional-only? Right now, this is looking like the optimal approach to reduce complexity not only in CustomParticle.__init__ and its docstring, but also in ParticleList and its attributes, particle_input, and the particle factory function.

namurphy · 2023-01-10T00:55:03Z

plasmapy/particles/_factory.py

+    try:
+        return Particle(*args, **kwargs)
+    except _particle_creation_exceptions:
+        # If the first argument is a string but is not a valid particle,
+        # then don't try to create a CustomParticle or ParticleList.
+        if args and isinstance(args[0], str):
+            raise
+
+    for particle_type in (CustomParticle, ParticleList):
+        with contextlib.suppress(*_particle_creation_exceptions):
            return particle_type(*args, **kwargs)


Hm...it's looking like I'll need to restructure this part again in #1867 to enable @particle_input to take values for Z that are real but not integers.

StanczakDominik · 2023-01-10T06:05:25Z

Can we instead hold off on this until we've discussed #1873 in more detail? I'm writing up some thoughts on it right now, and we do have a telecon today, right?

namurphy · 2023-01-10T20:17:27Z

The approach suggested by @StanczakDominik at today's telecon is to implement a pattern like:

class CustomParticle:
    def __init__(self, mass: u.kg, charge: u.C):
        # current broad-strokes signature of CustomParticle creator
        pass

    @classmethod
    def _from_positionals(cls, *args):
        # use this from @particleinput
        ... # figure out physical type of argument -> create a dict -> initialize particle
        d = dict(mass = mass_value_kg, charge = charge_value_C)
        return cls(**d)

I think I might try this in a separate PR...

namurphy added 2 commits January 6, 2023 14:38

Update _get_physical_type_dict

9772cb2

Update _get_physical_type_dict

5a4c1c7

namurphy added Breaking plasmapy.particles Related to the plasmapy.particles subpackage labels Jan 6, 2023

namurphy added the status: in progress label Jan 6, 2023

namurphy added 7 commits January 8, 2023 12:23

Fix type hint

91c1595

Fix possible IndexError

4092626

Change signature of CustomParticle

701dd82

Fix special condition

dfb5bd1

Fix docstring

4f46d1f

Update test

e3e2cbf

Make kwargs consistent with docstring

145455d

αβtize!

namurphy removed the Breaking label Jan 8, 2023

namurphy added 6 commits January 8, 2023 16:16

Add changelog entry

02557da

Expand examples section of docstring

5273770

Add a test for duplicate args & kwargs

0a8a0fa

Improve Quantity interoperability with ParticleList

220ad3d

Make ParticleList methods Quantity-compatible

dcf0fe2

Update changelog entries

64b05ec

namurphy changed the title ~~Change signature of CustomParticle~~ Change signature of CustomParticle and improve |ParticleList| compatibility with |Quantity| objects Jan 9, 2023

namurphy added 4 commits January 8, 2023 19:30

Add a length as an invalid particle

0a6d127

Fix test

bb3bbd1

Shorten changelog entry

2e02f3c

Fix problem with test parametrization

5fdc4a2

namurphy marked this pull request as ready for review January 9, 2023 03:12

namurphy requested a review from a team as a code owner January 9, 2023 03:12

namurphy removed the request for review from a team January 9, 2023 03:12

namurphy requested a review from StanczakDominik January 9, 2023 03:12

StanczakDominik requested changes Jan 9, 2023

View reviewed changes

namurphy added this to the 2023.1.0 milestone Jan 9, 2023

namurphy added 5 commits January 9, 2023 18:39

Use extract method refactoring pattern

430217a

Refactor particle factory

3f1e41a

Fix type hint

c5995c0

Fix test

09381e6

Fix test

307ee75

namurphy commented Jan 10, 2023

View reviewed changes

namurphy mentioned this pull request Jan 10, 2023

Add strict and allowed_physical_types keyword arguments to _get_physical_type_dict #1880

Merged

namurphy marked this pull request as draft January 10, 2023 21:55

namurphy added status: on hold Issues & PRs that are being intentionally delayed and removed status: in progress labels Jan 10, 2023

This was referenced Jan 10, 2023

Add separate constructor for CustomParticle #1881

Merged

Apply new CustomParticle constructor to particle factory #1884

Merged

namurphy closed this Jan 12, 2023

namurphy mentioned this pull request Mar 4, 2023

Make ParticleList accept Quantity arrays #1987

Merged

namurphy removed the status: on hold Issues & PRs that are being intentionally delayed label May 23, 2023

namurphy deleted the custom-particle-signature branch May 24, 2023 16:15

namurphy mentioned this pull request May 26, 2023

Make ParticleList compatible with Quantity objects with physical type of mass or electrical charge #1872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add separate constructor for `CustomParticle` and improve |ParticleList| compatibility with |Quantity| objects #1874

Add separate constructor for `CustomParticle` and improve |ParticleList| compatibility with |Quantity| objects #1874

namurphy commented Jan 6, 2023 •

edited

github-actions bot commented Jan 6, 2023

namurphy commented Jan 6, 2023

codecov bot commented Jan 8, 2023 •

edited

StanczakDominik left a comment

StanczakDominik Jan 9, 2023

namurphy Jan 9, 2023

StanczakDominik Jan 9, 2023

namurphy Jan 9, 2023

StanczakDominik Jan 9, 2023

namurphy Jan 9, 2023

StanczakDominik Jan 9, 2023

namurphy Jan 9, 2023

namurphy commented Jan 9, 2023 •

edited

namurphy Jan 10, 2023

StanczakDominik commented Jan 10, 2023

namurphy commented Jan 10, 2023

Add separate constructor for CustomParticle and improve |ParticleList| compatibility with |Quantity| objects #1874

Add separate constructor for CustomParticle and improve |ParticleList| compatibility with |Quantity| objects #1874

Conversation

namurphy commented Jan 6, 2023 • edited

github-actions bot commented Jan 6, 2023

namurphy commented Jan 6, 2023

codecov bot commented Jan 8, 2023 • edited

Codecov Report

StanczakDominik 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

namurphy commented Jan 9, 2023 • edited

Choose a reason for hiding this comment

StanczakDominik commented Jan 10, 2023

namurphy commented Jan 10, 2023

Add separate constructor for `CustomParticle` and improve |ParticleList| compatibility with |Quantity| objects #1874

Add separate constructor for `CustomParticle` and improve |ParticleList| compatibility with |Quantity| objects #1874

namurphy commented Jan 6, 2023 •

edited

codecov bot commented Jan 8, 2023 •

edited

namurphy commented Jan 9, 2023 •

edited