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

Make common_isotopes, known_isotopes, and stable_isotopes each return a ParticleList #2559

Merged

Conversation

namurphy
Copy link
Member

@namurphy namurphy commented Mar 5, 2024

Whilst working on #2458, I noticed that a few of the functions in plasmapy.particles.atomic each returned a list containing Particle objects. This PR changes it so that they return a ParticleList.

@github-actions github-actions bot added the plasmapy.particles Related to the plasmapy.particles subpackage label Mar 5, 2024
@github-actions github-actions bot added testing changes existing API Involves or proposes breaking (backward incompatible) changes labels Mar 5, 2024
@@ -664,11 +664,11 @@ def common_isotopes_for_element(
CommonIsotopes = [
isotope
for isotope in isotopes
if "abundance" in _isotopes.data_about_isotopes[isotope]
if "abundance" in _isotopes.data_about_isotopes[isotope.isotope]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Particle.isotope returns a string with the symbol of the isotope, which is expected by that dict.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we didn't return dict objects?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dict here was a private one containing isotope data, so it wasn't part of our public API. I'd say that there are times when a dict is a good choice for a return value, and other times when it's worth creating something like a class or namedtuple. As with everything, there are tradeoffs!

plasmapy/particles/atomic.py Outdated Show resolved Hide resolved
Comment on lines -486 to -488
isotopic_abundance_elements = (
atomic_number(atomic_numb) for atomic_numb in range(1, 119)
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to have discovered Python generators in Dec 2017 and perhaps maybe had just a little bit too much fun with them...

@namurphy namurphy marked this pull request as ready for review March 5, 2024 00:55
@namurphy namurphy requested a review from a team as a code owner March 5, 2024 00:55
@namurphy namurphy requested review from ejohnson-96 and removed request for a team March 5, 2024 00:55
@namurphy namurphy changed the title Make common_isotopes, known_isotopes, and stable_isotopes return a ParticleList Make common_isotopes, known_isotopes, and stable_isotopes each return a ParticleList Mar 5, 2024
@namurphy namurphy added the status: ready for review PRs that are ready for code review label Mar 5, 2024
Copy link
Contributor

@ejohnson-96 ejohnson-96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@namurphy
Copy link
Member Author

namurphy commented Mar 6, 2024

pre-commit.ci autofix

@namurphy namurphy enabled auto-merge (squash) March 6, 2024 23:46
@namurphy namurphy merged commit f44ef1b into PlasmaPy:main Mar 6, 2024
15 checks passed
@github-actions github-actions bot removed the status: ready for review PRs that are ready for code review label Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes existing API Involves or proposes breaking (backward incompatible) changes plasmapy.particles Related to the plasmapy.particles subpackage testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants