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

Add `isotope_name` attribute on `Particle` instance #510

Merged
merged 2 commits into from Jul 17, 2018

Conversation

Projects
None yet
4 participants
@ritiek
Copy link
Contributor

commented Jul 10, 2018

Closes #503.

If we want to return deuterium and tritium for those special cases, maybe we should make appropriate changes to element_name property since isotope_name just returns formatted output of element_name (and mass_number)? Or do we want to add these checks specifically to isotope_name so that element_name stays as is?

I haven't added any checks for these special cases yet, so at the moment it looks like this:

>>> deuterium = Particle("D+")
>>> deuterium.element_name
'hydrogen'
>>> deuterium.isotope_name
'hydrogen-2'
@codecov

This comment has been minimized.

Copy link

commented Jul 10, 2018

Codecov Report

Merging #510 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
+ Coverage   98.02%   98.02%   +<.01%     
==========================================
  Files          44       44              
  Lines        3690     3702      +12     
==========================================
+ Hits         3617     3629      +12     
  Misses         73       73
Impacted Files Coverage Δ
plasmapy/atomic/particle_class.py 100% <100%> (ø) ⬆️

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 d22b5a2...bd7a0fa. Read the comment docs.

@StanczakDominik
Copy link
Member

left a comment

I think isotope_name would be the better place for deuterium and tritium to pop up. When you're asking specifically about "what element is deuterium?", you don't want to say "hydrogen!", as that's not quite right.

The code looks good up until now!

@ritiek

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2018

Okay, got it.

@ritiek ritiek force-pushed the ritiek:isotope-name branch from ae31465 to bd7a0fa Jul 11, 2018

@namurphy namurphy added the Atomic label Jul 17, 2018

@namurphy
Copy link
Member

left a comment

I have one question, and once that's resolved I'm happy for this to be merged.

@@ -196,6 +203,7 @@
'particle': 'H-1 0+',
'element': 'H',
'isotope': 'H-1',
'isotope_name': 'hydrogen-1',

This comment has been minimized.

Copy link
@namurphy

namurphy Jul 17, 2018

Member

There are two names that we could adopt for H-1: 'hydrogen-1' and 'protium'. Do we have a preference between the two? Using 'protium' would be more consistent with the way we use 'deuterium' and 'tritium', but 'protium' is less well known.

This comment has been minimized.

Copy link
@namurphy

namurphy Jul 17, 2018

Member

I also like protium because we can use it to name Star Wars villains, e.g., Darth Protium.

This comment has been minimized.

Copy link
@StanczakDominik

StanczakDominik Jul 17, 2018

Member

I like hydrogen-1 here, I don't think I've ever heard "protium" used. That's not to say I've heard much.

@SolarDrew

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

LGTM 👍

@StanczakDominik StanczakDominik merged commit 0a5eb2e into PlasmaPy:master Jul 17, 2018

5 checks passed

ci/circleci: test-html Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 98.02%)
Details
codecov/project 98.02% (+<.01%) compared to d22b5a2
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:isotope-name branch Jul 17, 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.