-
Notifications
You must be signed in to change notification settings - Fork 312
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
Issue 285: Add ionization energy data from NIST #2657
Conversation
Thank you for submitting a pull request (PR) to PlasmaPy! ✨ The future of the project depends on contributors like you, so we deeply appreciate it! 🌱 Our contributor guide has information on:
Important PlasmaPy recently switched to an The bottom of this page shows several checks that are run for every PR. Don't worry if something broke! We break stuff all the time. 😺 Click on "Details" to learn why a check didn't pass. Please also feel free to ask for help. We do that all the time as well. 🌸 You can find us in our chat room or weekly community meeting & office hours. Here are some tips:
If this PR is marked as ready for review, someone should stop by to provide a code review and offer suggestions soon. ✅ If you don't get a review within a few days, please feel free to send us a reminder. Please also use SI units within PlasmaPy, except when there is strong justification otherwise or in some examples. We thank you once again! |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2657 +/- ##
==========================================
+ Coverage 95.21% 95.23% +0.01%
==========================================
Files 103 104 +1
Lines 9412 9435 +23
Branches 2155 2159 +4
==========================================
+ Hits 8962 8985 +23
Misses 273 273
Partials 177 177 ☔ View full report in Codecov by Sentry. |
pre-commit.ci autofix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice start! Here are a few minor corrections.
There is one larger bug: right now, this code still returns the ground state ionization energy even if the particle is an ion, e.g.
Particle('C +2').ionization_energy == Particle('C +0').ionization_energy
>True
while, according to the NIST tables, the ionization states above should be 47 eV and 11 eV respectively. I can see two ways of fixing this:
-
Make this property only available for particles corresponding to neutral atoms
-
Much more ambitious: implement this property for all of the ion charge states!
If you chose the second path, I would recommend the following:
- Download the data for every element in the NIST database as a CSV
- Write a python script that takes that data and puts it into a single json file that you can add to the particles/data folder. You could add different isotopes there too.
- In the attribute, determine the isotope, charge, and nucleus of the particle and return the appropriate value from your json file.
No pressure to go the second path if you don't want to, just adding this for neutral atoms would be a nice start! But thought I'd offer it as a suggestion since you seem to have handled this pretty easily :)
Co-authored-by: Peter Heuer <pvheuer@gmail.com>
Co-authored-by: Peter Heuer <pvheuer@gmail.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Merge conflict resolve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks for adding the downloader script!
One remaining bug I found: you have separate data for deuterium (H-2) and hydrogen (H-1), but it looks like with the current logic both of these get interpreted as 'Hydrogen' so you never actually get the value for Deuterium. It's only slightly different, but sine you have the data in there it would be good to have it return the right one!
You could maybe add one more test for for this exception (since in all the other cases, you do have a test that correctly shows that the different isotopes have equal ionization energies).
To be clear on the physics, technically all of the isotopes have different ionization energies, but the difference is just vanishingly small above He so it's not measurable.
Particle('H').ionization_energy == Particle('D').ionization_energy == Particle('H-2').ionization_energy
>True
Should return False
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @chasepd , thanks so much for putting this together!
Description
Adds ionization energy data from NIST to the particle data json files, as well as a property in the
Particle
class to retrieve this information. Returns a descriptiveMissingParticleDataError
if ionization energy data is not available for the particle.Motivation and context
Addresses the requested feature at: #285.
Related issues
#285