-
Notifications
You must be signed in to change notification settings - Fork 306
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 2673: Rename binding_energy
to remove ambiguity, add electron_binding_energy
#2692
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: Peter Heuer <pvheuer@gmail.com>
Co-authored-by: Peter Heuer <pvheuer@gmail.com>
Co-authored-by: Peter Heuer <pvheuer@gmail.com>
Co-authored-by: Peter Heuer <pvheuer@gmail.com>
Co-authored-by: Peter Heuer <pvheuer@gmail.com>
Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2692 +/- ##
==========================================
+ Coverage 94.90% 95.23% +0.32%
==========================================
Files 104 104
Lines 9435 9435
Branches 2159 2159
==========================================
+ Hits 8954 8985 +31
+ Misses 299 273 -26
+ Partials 182 177 -5 ☔ View full report in Codecov by Sentry. |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
@chasepd This looks great! Unfortunately something really strange is happening with Git here (probably my fault): when I tried to pull this branch locally, GitHub Desktop ended up pushing I think the issue might be that it looks like you are editing the code on your main branch, then opening PRs from that. Typically it's better to make a new branch for each PR (from main), then delete it after the PR is merged. Probably wouldn't be an issue if I was better at Git! EDIT: Just to reassure you nothing is lost, I'm easily able to create a new branch off of your last commit, which I've done here: I just can't figure out how to reset THIS branch. I think only you may be able to do that, since it is on your repository. |
No worries! I have a local copy, I'll just nuke this PR and my local fork, re-fork this, and then make a new PR from a non-main branch |
Description
Adds requested feature from #2673.
binding_energy
tonuclear_binding_energy
, but leavesbinding_energy
as a pointer tonuclear_binding_energy
for backwards compatibility with a warning that it is deprecated and will be removed in a future release.electron_binding_energy
, defined as the sum ofionization_energy
for all charge states above the given ionization.Motivation and context
See #2673. With the addition of ionization energy data, calculating
electron_binding_data
becomes fairly easy. However, this causesbinding_energy
to be ambiguous/confusing, and is renamed tonuclear_binding_energy
for clarity and greater specificity.Related issues
Resolves #2673.