Reorganize and refactor atomic subpackage #122

namurphy · 2017-09-26T17:59:46Z

This pull request makes the following changes to the atomic subpackage:

Rename element_symbol to atomic_symbol
Rename energy_from_nuclear_reaction to nuclear_reaction_energy
Move nuclear_reaction_energy and nuclear_binding_energy to new file called nuclear.py
Clean up and reorganize docstrings in atomic.py
Improve variable name clarity and consistency in atomic.py
Change atomic_symbols_list to a dictionary called atomic_symbols in elements.py
Use warnings.warn when there is a UserWarning instead of raise
Disallow neutrons in atomic_symbol and element_name, but continue to allow them in isotope_symbol, isotope_mass, and mass_number because neutrons do count as nuclides
Add helper functions to identify atomic special cases, including:
- __is_hydrogen
- __is_positron
- __is_electron
- __is_antiproton
- __is_neutron
- __is_alpha

This commit enforces the following naming conventions: element = element_symbol(argument) isotope = isotope_symbol(argument) Using `symbol` leaves it ambiguous whether it refers to an element, isotope, or ion.

The new atomic_symbols dictionary in elements.py has atomic numbers as keys and the atomic symbols as items. This results in the same functionality, but will not break if we remove neutrons from index 0.

New functions include: __is_hydrogen, __is_positron, __is_electron, __is_antiproton, __is_neutron, and __is_alpha. These functions check whether or not a particle string corresponds to any of the cases with special names. These functions are designed to make functions like atomic_symbol more readable. This is still a work in progress.

pep8speaks · 2017-09-26T17:59:54Z

Hello @namurphy! Thanks for updating the PR.

In the file plasmapy/atomic/tests/test_atomic.py, following are the PEP8 issues :

Line 844:26: E127 continuation line over-indented for visual indent

Comment last updated on September 29, 2017 at 21:52 Hours UTC

This fixes a bug where __extract_charge_state('H-1-') did not return ('H-1', -1).

Up until now, atomic symbols were case-insensitive (e.g., 'd' and 'D' both stand for deuterium; but 'p' means proton and 'P' means phosphorus). The problem with case-insensitivity is that it allows for sloppier and less consistent code.

Because: "coverage/coveralls — Coverage decreased (-0.01%) to 98.955%"

StanczakDominik

Found some mostly minor text fixes to be done. :)

StanczakDominik · 2017-09-27T15:57:40Z

.gitignore

+Untitled*
+untitled*
+C\:*


... Wait, what? C:?

For some reason on my workstation running CentOS 6.8 I occasionally get files named C:\\nppdf32Log\\debuglog.txt for some unbeknownst reason. Hm...maybe *debuglog* would be a better choice, in case this has any adverse impact on Windows.

Huh, that's weird.

Here's something I found out about recently that might be helpful, though: https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

StanczakDominik · 2017-09-27T16:01:42Z

plasmapy/atomic/nuclear.py

+    -------
+    energy: Quantity
+        The change in nuclear binding energy, which will be positive
+        if the reaction releases and negative if the reaction is


...releases energy and...

StanczakDominik · 2017-09-27T16:03:24Z

plasmapy/atomic/nuclear.py

+                if item == 'n':
+                    symbol = 'n'
+                else:
+                    symbol = isotope_symbol(item)


I'm still not sure what's worse, the weird feeling I get seeing how this has to be handled separately, or calling a neutron an isotope.

Good catch. Since isotope_symbol works with neutrons, we can shorten these four lines down to just symbol = isotope_symbol(item). I may change symbol to isotope for clarity and consistency as well.

StanczakDominik · 2017-09-27T16:07:42Z

plasmapy/atomic/atomic.py

+    if __is_neutron(argument) and (mass_numb is None or mass_numb == 1):
+        return 'n'
+    elif argument == 0 and mass_numb == 1:
+        return 'n'


Oh, so you did add it! One of my previous comments may have just been invalidated, then!

StanczakDominik · 2017-09-27T16:10:45Z

plasmapy/atomic/atomic.py

-            else:
+                warn("Redundant mass number information in isotope_symbol from"
+                     " inputs: (" + str(argument)+", " + str(mass_numb) + ")",
+                     UserWarning)


Excuse me, do you have a moment to talk about our typing savior the f-string?

f"Redundant mass number information in isotope_symbol from inputs: ({argument}, {mass_numb})",

Oh, great idea! I think I had some bad experiences using Fortran formatting (i.e., I used Fortran formatting) and so have been reluctant to learn how to do it in Python.

StanczakDominik · 2017-09-27T16:13:24Z

plasmapy/atomic/atomic.py

@@ -434,24 +444,25 @@ def half_life(argument, mass_numb=None):
            half_life_sec = Isotopes[isotope]['half_life']
    except Exception:
        half_life_sec = None
-        raise UserWarning("The half-life for isotope " + isotope +
-                          " is not available; returning None.")
+        warn("The half-life for isotope " + isotope + " is not available; "


Once more, a proper place for the glory of f-strings!

StanczakDominik · 2017-09-27T16:13:48Z

plasmapy/atomic/atomic.py


    Returns
    -------
    mass_number : integer
-       An integer representing the mass number of the isotope.
+       The total number of protons plus neutrons in a nuclide.


StanczakDominik · 2017-09-27T16:17:12Z

plasmapy/atomic/atomic.py

-    uses hydrogen's standard atomic weight based on terrestrial values.  To
-    get the mass of a proton, use ion_mass('p').
+    Calling ion_mass('H') does not return the mass of a proton but
+    instea uses hydrogen's standard atomic weight based on terrestrial


typo, instead

StanczakDominik · 2017-09-27T16:25:50Z

plasmapy/atomic/atomic.py

-        A string or integer representing an atomic number or element, or a
-        string represnting an isotope.
+        A string or integer representing an atomic number or element,
+        or a string represnting an isotope.

    unstable_instead: boolean


Actually, how about just unstable?

I've been going back and forth about how best to name this. The possibilities I've been wondering about are:

stable_isotopes('Fe', unstable_instead=True) (status quo)

stable_isotopes('Fe', unstable=True) (your suggestion)

stable_isotopes('Fe', invert=True)

unstable_isotopes('Fe') (create a new function)

Not provide this functionality and let people do a list comprehension like:

[isotope for isotope in known_isotopes('H') if isotope not in stable_isotopes('H')]

Are any of these better than the others? The most readable is unstable_isotopes('Fe'), though I'm wondering if we need to create a function for it.

StanczakDominik · 2017-09-27T16:28:03Z

plasmapy/atomic/atomic.py


    return Z


+def electric_charge(argument):


StanczakDominik · 2017-09-27T17:48:24Z

Invert is less direct than stable. Another function is less simple. Unstable instead seems lengthy.

…

On Sep 27, 2017 19:42, "Nick Murphy" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In plasmapy/atomic/atomic.py <#122 (comment)>: > Parameters ---------- argument: integer or string - A string or integer representing an atomic number or element, or a - string represnting an isotope. + A string or integer representing an atomic number or element, + or a string represnting an isotope. unstable_instead: boolean I've been going back and forth about how best to name this. The possibilities I've been wondering about are: 1. stable_isotopes('Fe', unstable_instead=True) (status quo) 2. stable_isotopes('Fe', unstable=True) (your suggestion) 3. stable_isotopes('Fe', invert=True) 4. unstable_isotopes('Fe') (create a new function) 5. Not provide this functionality and let people do a list comprehension like: [isotope for isotope in known_isotopes('H') if isotope not in stable_isotopes('H')] Are any of these better than the others? The most readable is unstable_isotopes('Fe'), though I'm wondering if we need to create a function for it. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#122 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKxDLysdUe_SgTSNNJ719zL9iiARR65hks5smokKgaJpZM4PkpW9> .

namurphy · 2017-09-27T18:29:07Z

I'm thinking about eventually creating a decorator that will take the arguments for these various functions and parse them into the standardized form expected later on. That will probably simplify a lot of this code. I think I may save that for a future pull request though.

namurphy added 9 commits September 25, 2017 14:46

Rename element_symbol to atomic_symbol

638fdd9

Rename function to nuclear_reaction_energy

fd38f05

Move nuclear functions to new file

0d96339

Clean up atomic docstrings with minor reorganization

6338e32

Improve variable names consistency in atomic.py

2b3b03f

This commit enforces the following naming conventions: element = element_symbol(argument) isotope = isotope_symbol(argument) Using `symbol` leaves it ambiguous whether it refers to an element, isotope, or ion.

Change atomic_symbols_list to a dict

55bdce5

The new atomic_symbols dictionary in elements.py has atomic numbers as keys and the atomic symbols as items. This results in the same functionality, but will not break if we remove neutrons from index 0.

Use warnings.warn instead of raise for UserWarnings

49da70c

Disallow neutrons in atomic_symbol & element_name

843bde9

namurphy added the plasmapy.particles Related to the plasmapy.particles subpackage label Sep 26, 2017

namurphy added this to the v0.1 milestone Sep 26, 2017

namurphy requested a review from StanczakDominik September 26, 2017 17:59

namurphy added 10 commits September 26, 2017 14:05

Extend .gitignore

62ce9ef

Protect integrity of spacetime through PEP 8 compliance

ca88ad4

Test and fix __extract_charge_state

519c432

This fixes a bug where __extract_charge_state('H-1-') did not return ('H-1', -1).

Add tests for __is_hydrogen in atomic subpackage

a6928e1

Improve test coverage and fix bugs in atomic.py

2efa711

Test for case-sensitive atomic symbols

aadba67

Up until now, atomic symbols were case-insensitive (e.g., 'd' and 'D' both stand for deuterium; but 'p' means proton and 'P' means phosphorus). The problem with case-insensitivity is that it allows for sloppier and less consistent code.

Enforce case-sensitive atomic symbols

ef1242a

Improve exception handling in atomic.py

8118d9a

Improve test coverage in atomic.py

4bc1f93

Improve test coverage for atomic routines

27cee26

Because: "coverage/coveralls — Coverage decreased (-0.01%) to 98.955%"

StanczakDominik requested changes Sep 27, 2017

View reviewed changes

namurphy added 2 commits September 29, 2017 17:51

Update .gitignore

469fb35

Clean up code in atomic subpackage

78ca910

namurphy merged commit 7f06dd7 into PlasmaPy:master Sep 29, 2017

namurphy mentioned this pull request Oct 3, 2017

Improve functionality of atomic subpackage #48

Closed

3 tasks

StanczakDominik added this to Done in Atomic physics Oct 3, 2017

namurphy mentioned this pull request Nov 23, 2017

Refactor nuclear_binding_energy and nuclear_reaction_rate #169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize and refactor atomic subpackage #122

Reorganize and refactor atomic subpackage #122

namurphy commented Sep 26, 2017

pep8speaks commented Sep 26, 2017 •

edited

StanczakDominik left a comment

StanczakDominik Sep 27, 2017

namurphy Sep 27, 2017 •

edited

StanczakDominik Sep 27, 2017

StanczakDominik Sep 27, 2017

StanczakDominik Sep 27, 2017

namurphy Sep 27, 2017 •

edited

StanczakDominik Sep 27, 2017

StanczakDominik Sep 27, 2017

namurphy Sep 27, 2017

StanczakDominik Sep 27, 2017

StanczakDominik Sep 27, 2017

StanczakDominik Sep 27, 2017

StanczakDominik Sep 27, 2017

namurphy Sep 27, 2017

StanczakDominik Sep 27, 2017

StanczakDominik commented Sep 27, 2017 via email

namurphy commented Sep 27, 2017

Reorganize and refactor atomic subpackage #122

Reorganize and refactor atomic subpackage #122

Conversation

namurphy commented Sep 26, 2017

pep8speaks commented Sep 26, 2017 • edited

Comment last updated on September 29, 2017 at 21:52 Hours UTC

StanczakDominik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namurphy Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namurphy Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanczakDominik commented Sep 27, 2017 via email

namurphy commented Sep 27, 2017

pep8speaks commented Sep 26, 2017 •

edited

namurphy Sep 27, 2017 •

edited

namurphy Sep 27, 2017 •

edited