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

Rename integer charge to charge number #1136

Merged
merged 36 commits into from
May 18, 2021
Merged

Conversation

namurphy
Copy link
Member

This PR replaces usage of the term "integer charge" and code elements named like integer_charge with the term "charge number" and charge_number. This is because charge number is the technical name for this concept (see #1126). The things named after "integer charge" are going to be kept for a few releases and then removed.

As part of this PR, I'm likely going to use a variant of the astropy.utils.decorators.deprecated decorator.

Closes #1126. Closes #945.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added plasmapy.diagnostics Related to the plasmapy.diagnostics subpackage plasmapy.formulary Related to the plasmapy.formulary subpackage notebooks Related to example Jupyter notebooks in docs/examples/ plasmapy.particles Related to the plasmapy.particles subpackage labels May 11, 2021
@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #1136 (bfaeac6) into master (68801c2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1136      +/-   ##
==========================================
+ Coverage   96.90%   96.92%   +0.01%     
==========================================
  Files          70       71       +1     
  Lines        6953     6992      +39     
==========================================
+ Hits         6738     6777      +39     
  Misses        215      215              
Impacted Files Coverage Δ
plasmapy/particles/__init__.py 100.00% <ø> (ø)
plasmapy/particles/symbols.py 100.00% <ø> (ø)
plasmapy/utils/__init__.py 100.00% <ø> (ø)
plasmapy/diagnostics/thomson.py 100.00% <100.00%> (ø)
plasmapy/dispersion/two_fluid_dispersion.py 100.00% <100.00%> (ø)
plasmapy/formulary/collisions.py 97.19% <100.00%> (ø)
plasmapy/formulary/parameters.py 99.09% <100.00%> (ø)
plasmapy/formulary/radiation.py 100.00% <100.00%> (ø)
plasmapy/particles/atomic.py 100.00% <100.00%> (ø)
plasmapy/particles/decorators.py 100.00% <100.00%> (ø)
... and 12 more

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 68801c2...bfaeac6. Read the comment docs.

@github-actions github-actions bot added the plasmapy.utils Related to the plasmapy.utils subpackage label May 12, 2021
Copy link
Member

@StanczakDominik StanczakDominik left a comment

Choose a reason for hiding this comment

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

LGTM in general; test failures seem to point to a mis-import probably since 5d2d129; the doc failure is unrelated (see Element). A couple of docstring mishaps in there.

docs/particles/ionization_states.rst Outdated Show resolved Hide resolved
docs/particles/particle_class.rst Outdated Show resolved Hide resolved
...since changes to integer_charge should be made known to the
end user.
...and use ReST markup for ionic notation while I'm at it.
@namurphy
Copy link
Member Author

I'm not sure why sections of docstrings ended up getting repeated. It could have been a mishap with auto-renaming via PyCharm, though it could have been a side effect of coding while listening to a BBC dramatization of my favorite book series. 🐉 In any case, I'll need to double check for this problem before this gets merged, and I have a few other things I need to finish up too. Thanks for finding that!

@namurphy
Copy link
Member Author

namurphy commented May 12, 2021

My to do list for this PR and beyond. It's mostly tedious work at this point.

  • Add back stand-alone integer_charge function and deprecate it
  • Test the integer_charge function
  • Add back the IonizationState.integer_charges and deprecate it
  • Test IonizationState.integer_charges
  • Add back ParticleList.integer_charges and deprecate it
  • Test ParticleList.integer_charges
  • Check the files changed to make sure I didn't add in excessive extra redundant redundancies again.
  • Add a change log entry
  • Create an issue to remove deprecated features from plasmapy.particles and label it with the milestone for, say, 0.9.0
  • Create an issue in PlasmaPy-NEI repo to update all of this

@StanczakDominik
Copy link
Member

Probably don't need to have exhaustive tests for integer_charge if it's just going to be a deprecated alias for charge_number!

@namurphy
Copy link
Member Author

Probably don't need to have exhaustive tests for integer_charge if it's just going to be a deprecated alias for charge_number!

Very true. I was planning on having maybe one or two tests for each function/method...just enough to make sure it works.

Copy link
Member

@rocco8773 rocco8773 left a comment

Choose a reason for hiding this comment

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

Add a stub file in docs/api_static for plasmapy.utils.decorators.deprecation.

namurphy and others added 2 commits May 17, 2021 20:46
Comment on lines +788 to +790
def test_integer_charge():
with pytest.warns(PlasmaPyFutureWarning):
assert integer_charge("Fe 20+") == 20
Copy link
Member

Choose a reason for hiding this comment

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

If you want to be thorough, then it would be worthwhile to do a mock test here to ensure integer_charge calls charge_number. It's a bit overkill, but it does enforce the connection between the two.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's probably more effort than is worth it for a deprecated feature, though this idea is worth keeping in mind for future features that we intend to keep around for a while.

@rocco8773
Copy link
Member

rocco8773 commented May 18, 2021

Do you want to merge #1073 first because it sure looks like you branched this PR off of that one? If so, that would avoid the scope creep of this PR.

@namurphy
Copy link
Member Author

Do you want to merge #1073 first because it sure looks like you branched this PR off of that one? If so, that would avoid the scope creep of this PR.

I'd rather merge this one first and then go back to #1073 and enjoy the wonders of resolving git conflicts. Thanks for the quick review!

(Also: it's amazing how productive I can be when I'm procrastinating writing emails that I don't want to write.)

@namurphy namurphy enabled auto-merge (squash) May 18, 2021 03:40
@StanczakDominik StanczakDominik dismissed rocco8773’s stale review May 18, 2021 05:37

Looks like the stub file is in there, and I didn't see any more issues being raised!

@namurphy namurphy merged commit d5859dc into PlasmaPy:master May 18, 2021
@namurphy namurphy deleted the charge_number branch May 18, 2021 14:16
@rocco8773 rocco8773 mentioned this pull request Dec 10, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebooks Related to example Jupyter notebooks in docs/examples/ plasmapy.diagnostics Related to the plasmapy.diagnostics subpackage plasmapy.formulary Related to the plasmapy.formulary subpackage plasmapy.particles Related to the plasmapy.particles subpackage plasmapy.utils Related to the plasmapy.utils subpackage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate integer_charge in favor of charge_number Create @deprecated decorator based off of Astropy's
3 participants