Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept tuple or list of corresponding particles or atomic numbers in `particle_input` #483

Merged
merged 11 commits into from Jun 9, 2018

Conversation

Projects
None yet
4 participants
@ritiek
Copy link
Contributor

commented Jun 2, 2018

Addresses #336. Now particle_input can take in a tuple or a list of particles or atomic numbers.

>>> from plasmapy.atomic import Particle, particle_input

>>> @particle_input
>>> def happy_function(particles: (Particle, Particle)):
...    print(particles[0].integer_charge)
...    print(particles[1].integer_charge)

>>> happy_function(('e-', 'p+'))
-1
1

>>> @particle_input
>>> def cute_function(particles: [Particle]):
...    for particle in particles:
...        print(particle.integer_charge)

>>> cute_function(('e-', 'p+', 'alpha'))
-1
1
2

It raises a TypeError when input parameters do not match expected parameters, for example:

>>> @particle_input
>>> def sad_function(particles: (Particle, Particle, Particle)):
...    print(particles[0].integer_charge)
...    print(particles[1].integer_charge)
...    print(particles[2].integer_charge)

>>> sad_function(('e-', 'p+'))
TypeError: Number of parameters allowed in the tuple (3 parameters) are not equal to number of parameters passed in the tuple (2 parameters).

>>> @particle_input
>>> def unlucky_function(particles: [Particle, Particle]):
...    print(particles[0].integer_charge)
...    print(particles[1].integer_charge)

>>> unlucky_function(('e-', 'p+'))
TypeError: A list can take any number of parameters without explictly mentioning the number of parameters in declaration. Use a tuple instead or '[Particle]' should be enough.

If this looks good enough, I can start writing tests for them!

namurphy and others added some commits Apr 9, 2018

Test tuple annotations in @particle_input
This test checks that functions decorated by @particle_input with an
argument that is annotated with (Particle, Particle) will pass through
the expected Particle instances.  In essence, this test checks that
using this tuple annotation will allow @particle_input to behave in
pretty much the same was as having two separate arguments that are
each decorated with Particle.

When setting this test up, we have to be careful because running
Particle('p+') == 'p+' will return True because the __eq__ magic
method for the Particle class allows comparisons with string
representations of particles.
@codecov

This comment has been minimized.

Copy link

commented Jun 2, 2018

Codecov Report

Merging #483 into master will decrease coverage by 0.03%.
The diff coverage is 93.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #483      +/-   ##
==========================================
- Coverage   93.22%   93.18%   -0.04%     
==========================================
  Files          70       70              
  Lines        6790     6871      +81     
==========================================
+ Hits         6330     6403      +73     
- Misses        460      468       +8
Impacted Files Coverage Δ
plasmapy/atomic/particle_input.py 100% <100%> (ø) ⬆️
plasmapy/atomic/tests/test_particle_input.py 86.95% <83.33%> (-1.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8fddc9...15e330c. Read the comment docs.

ritiek added some commits Jun 6, 2018

@pep8speaks

This comment has been minimized.

Copy link

commented Jun 8, 2018

Hello @ritiek! Thanks for updating your pull request.

Congratulations! There are no PEP8 issues in this pull request. 😸

Comment last updated on June 09, 2018 at 13:21 Hours UTC

@ritiek ritiek force-pushed the ritiek:particle-input branch from e6f1c03 to ae0f246 Jun 8, 2018

@ritiek ritiek changed the title Accept tuple or list of corresponding particles or atomic numbers in `particle_input` [WIP] Accept tuple or list of corresponding particles or atomic numbers in `particle_input` Jun 8, 2018

ritiek added some commits Jun 8, 2018

@ritiek ritiek force-pushed the ritiek:particle-input branch from b6baa76 to 33a6cff Jun 9, 2018

@ritiek ritiek force-pushed the ritiek:particle-input branch from 33a6cff to f574d61 Jun 9, 2018

@ritiek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2018

I guess this is ready to merge. I'd be happy if someone can review it.

@StanczakDominik
Copy link
Member

left a comment

This is pretty neat. A couple minor text fixes would make this a bit easier to use, otherwise I'm happy to merge it!

def decorated_list_function(particles: [Particle]):
return particles
sample_particles = decorated_list_function(('Al 3+', 'C'))
sample_particles = decorated_list_function(['He', 'Ne', 'Ar'])

This comment has been minimized.

Copy link
@StanczakDominik

StanczakDominik Jun 9, 2018

Member

Oh, so THAT's what you meant there! I was trying to wrap my head around this yesterday 😆 Neat!

This comment has been minimized.

Copy link
@ritiek

ritiek Jun 9, 2018

Author Contributor

Yep, (Particle, Particle) accepts only 2 elements and (Particle, Particle, Particle) accepts 3 elements and so on.. (either as tuple or list).

while [Particle] accepts any number of Particle elements (either as tuple or list).

Rust programming language has a similar concept of passing vectors (think: lists in Python) and tuples of specific type to a function. I imagined it would be nice here as well. 😄

If the annotated argument is not a `str`, `int`, `tuple`, `list`
or `~plasmapy.atomic.Particle`; or if `Z` or `mass_numb` is
not an `int`; or if number of elements in a tuple do not match
the expected parameters; or if invalid list annotations in

This comment has been minimized.

Copy link
@StanczakDominik

StanczakDominik Jun 9, 2018

Member

Invalid list annotations sounds pretty unclear. However, I'm wondering whether we might be getting too verbose here... I think we can cut some of the description, if someone gets an exception they'll be looking at the code anyway, right?

This comment has been minimized.

Copy link
@ritiek

ritiek Jun 9, 2018

Author Contributor

I thought so too. If somebody gets an exception, they are told about the reason they are getting the exception. Maybe we should just have these

If the annotated argument is not a str, int, tuple, list or ~plasmapy.atomic.Particle; or if Z or mass_numb is not an int.

Not sure though.

This comment has been minimized.

Copy link
@StanczakDominik

StanczakDominik Jun 9, 2018

Member

That's beautiful. Put that in! :)

This comment has been minimized.

Copy link
@ritiek

ritiek Jun 9, 2018

Author Contributor

Okie!

expected_params = len(annotated_argnames)
if expected_params > 1:
raise TypeError(
f"A list can take any number of parameters "

This comment has been minimized.

Copy link
@StanczakDominik

StanczakDominik Jun 9, 2018

Member

Maybe "Put in [Particle] as the annotation to accept an arbitrary number of Particle arguments."

I love the idea though

This comment has been minimized.

Copy link
@ritiek

ritiek Jun 9, 2018

Author Contributor

Nice. That makes it easier to understand. I think maybe "arbitrary number of Particle elements" would be a better choice than "arbitrary number of Particle arguments". What do you think?

This comment has been minimized.

Copy link
@StanczakDominik

StanczakDominik Jun 9, 2018

Member

Nah, I think we should rather keep the 'elements' key word for iron, hydrogen etc here 😃

This comment has been minimized.

Copy link
@ritiek

ritiek Jun 9, 2018

Author Contributor

Haha, ok got it.


def test_invalid_list_type():
@particle_input
# This should be just [Particle] instead of [Particle, Particle]

This comment has been minimized.

Copy link
@StanczakDominik

StanczakDominik Jun 9, 2018

Member

Good testing

This comment has been minimized.

Copy link
@ritiek

ritiek Jun 9, 2018

Author Contributor

Thanks!

@ritiek ritiek force-pushed the ritiek:particle-input branch from 5c02dc4 to 15e330c Jun 9, 2018

@StanczakDominik
Copy link
Member

left a comment

Nice! Merging, thanks @ritiek :)

@StanczakDominik StanczakDominik merged commit de0d095 into PlasmaPy:master Jun 9, 2018

5 checks passed

ci/circleci: test-html Your tests passed on CircleCI!
Details
codecov/patch 93.27% of diff hit (target 93.22%)
Details
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +0.05% compared to b8fddc9
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ritiek ritiek deleted the ritiek:particle-input branch Jun 9, 2018

@namurphy namurphy changed the title [WIP] Accept tuple or list of corresponding particles or atomic numbers in `particle_input` Accept tuple or list of corresponding particles or atomic numbers in `particle_input` Jul 23, 2018

@namurphy namurphy added the Atomic label Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.