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.

Sign up for GitHub

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

Jump to bottom

Cleanup quantum.py docstrings #951

Merged
merged 13 commits into from
Feb 23, 2021
Merged

Cleanup quantum.py docstrings #951

merged 13 commits into from
Feb 23, 2021

Conversation

Tiger-Du
Copy link
Contributor

@Tiger-Du Tiger-Du commented Nov 14, 2020 •
edited
Loading

  • I have added a changelog entry for this pull request.
  • If adding new functionality, I have added tests and
    docstrings.
  • I have fixed any newly failing tests.

@codecov
Copy link

codecov bot commented Nov 14, 2020 •
edited
Loading

Codecov Report

Merging #951 (524126d) into master (46414cb) will increase coverage by 0.53%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #951      +/-   ##
==========================================
+ Coverage   96.29%   96.82%   +0.53%     
==========================================
  Files          62       69       +7     
  Lines        5715     6737    +1022     
==========================================
+ Hits         5503     6523    +1020     
- Misses        212      214       +2
Impacted Files Coverage Δ
plasmapy/formulary/quantum.py 67.05% <ø> (ø)
plasmapy/simulation/abstractions.py 76.04% <0.00%> (-1.74%) ⬇️
plasmapy/utils/pytest_helpers/pytest_helpers.py 91.46% <0.00%> (-1.22%) ⬇️
plasmapy/simulation/particletracker.py 98.27% <0.00%> (-0.06%) ⬇️
plasmapy/utils/__init__.py 100.00% <0.00%> (ø)
plasmapy/plasma/__init__.py 100.00% <0.00%> (ø)
plasmapy/formulary/drifts.py 100.00% <0.00%> (ø)
plasmapy/particles/atomic.py 100.00% <0.00%> (ø)
plasmapy/particles/nuclear.py 100.00% <0.00%> (ø)
plasmapy/particles/parsing.py 100.00% <0.00%> (ø)
... and 28 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 46414cb...524126d. Read the comment docs.

@Tiger-Du Tiger-Du mentioned this pull request Nov 14, 2020
Open
@rocco8773
Copy link
Member

I'm going to reply to your original #935 (comment) here to keep all the associated discussion in one location.

Hi, @rocco8773. I submitted a draft pull request (#951) that adds a section named Formula to the top of the docstring of one function, with the formula copied from Notes. Please let me know whether it's similar to what you had in mind and whether I should go ahead and add this section in the same way to all other functions in plasmapy.formulary.

Also, about the ":math:" parts: Do those render at some point into mathematical symbols? Or is ":math:" just a formatting convention for denoting mathematics in docstrings?

Yes, the :math: directive will be converted into symbols by sphinx when the documentation is built. You can find instructions on building documentation locally on our documentation site here.

You can also see the CI built version of the docs here, https://plasmapy--951.org.readthedocs.build/en/951/.

Also, we use numpydoc style docstrings, so creating non-standard sections will cause build errors. Unfortunately you're limited to the standard section names.

@Tiger-Du
Copy link
Contributor Author

Hi, @rocco8773. Thank you for your helpful comments. I made the change you suggested in addition to some formatting fixes in the most recent commit.

A question about the function deBroglie_wavelength: Should ValueError be included in the Raises section of the docstring?

And a question about formatting. I notice that there is some inconsistency in whether there is a blank line after the docstring of each function. According to PEP8, there should be no blank line, but since our docstrings are so long, I think an exception may be warranted. Either way, is this something we can automate with Black?

@rocco8773 rocco8773 self-assigned this Jan 19, 2021
rocco8773
rocco8773 requested changes Feb 18, 2021
View reviewed changes
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.

This is looking good, there are just a few minor things to do.

  1. Except for in the "Raises" section, whenever you see something like ~astropy.units.Quantity, ~plasmapy.particles.Particle, etc. they need to be surrounded by back ticks like `~astropy.units.Quantity`. This causes intersphinx to do the appropriate linking. (For this PR only worry about adding the back ticks ` to the de Broglie functionality and not all of quantum.py.)
  2. There are few more cases in the docstrings, warning messages, and raises messages that used 'deBroglie' instead of 'de Broglie'. Can you go through and update those cases? Thanks.

plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
@Tiger-Du Tiger-Du marked this pull request as ready for review February 20, 2021 05:44
rocco8773
rocco8773 requested changes Feb 20, 2021
View reviewed changes
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.

Looking good! Just a few more minor tweaks and it'll be there.

Can you review the "Warns" section in all the docstrings? Currently, most of them are not properly formatted. They should follow the pattern...

Warns
-----
: `~some.type`
    a proper desription

plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
plasmapy/formulary/quantum.py Outdated Show resolved Hide resolved
changelog/951.doc.rst Outdated Show resolved Hide resolved
@rocco8773 rocco8773 added docs PlasmaPy Docs at http://docs.plasmapy.org plasmapy.formulary Related to the plasmapy.formulary subpackage labels Feb 20, 2021
@rocco8773 rocco8773 changed the title Update quantum.py Cleanup quantum.py docstrings Feb 20, 2021
@Tiger-Du
Copy link
Contributor Author

@rocco8773 - Ok, I made the requested changes, and I think this PR is mergeable now. What do you think?

rocco8773
rocco8773 approved these changes Feb 23, 2021
View reviewed changes
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.

Alright, looks good to me! Thank you for making these improvements!

@rocco8773 rocco8773 merged commit 1503043 into PlasmaPy:master Feb 23, 2021
@StanczakDominik StanczakDominik added this to the 0.6.0 milestone Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StanczakDominik StanczakDominik StanczakDominik left review comments

@rocco8773 rocco8773 rocco8773 approved these changes

Assignees

@rocco8773 rocco8773

Labels
docs PlasmaPy Docs at http://docs.plasmapy.org plasmapy.formulary Related to the plasmapy.formulary subpackage
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Tiger-Du @rocco8773 @StanczakDominik