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

Fix incorrect docstrings #1369

Merged
merged 34 commits into from
Dec 18, 2020
Merged

Conversation

KevinCawley
Copy link
Contributor

@KevinCawley KevinCawley commented Dec 6, 2020

Changes were made to .py files to properly display in the TARDIS API.

Description

There were several instances where the formatting of docstrings was incorrect that was causing a breakage in the API. There were also a few LaTeX equations that were not properly formatted, so that was also updated as well. Another formatting error was fixed where it was inconsistent in the lines due to the docstring, which was resolved by donig \n at the start to get uniform documentation and keep with the current format. In addition, there was a small update to the montecarlo/montecarlo_numba folder as some formatting was missed that needed to be fixed.

There is an issue with a YAML creating a text-wall, that will result in a new issue.

Motivation and Context

Quality of Life, as well as making the API consistent and easily readable. Fixes #1154

How Has This Been Tested?

Docs were compiled locally by adding a line before line 80 in tardis/docs/conf.py (nbsphinx_allow_errors = True), and then running "python setup.py build_docs". This change in tardis/docs/conf.py was reverted rigth before this PR.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have assigned/requested two reviewers for this pull request.

@codecov
Copy link

codecov bot commented Dec 6, 2020

Codecov Report

Merging #1369 (0ea994d) into master (23ac153) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1369   +/-   ##
=======================================
  Coverage   72.38%   72.38%           
=======================================
  Files          66       66           
  Lines        5098     5098           
=======================================
  Hits         3690     3690           
  Misses       1408     1408           
Impacted Files Coverage Δ
tardis/tardis/io/util.py 71.76% <0.00%> (ø)
tardis/tardis/io/decay.py 89.65% <0.00%> (ø)
tardis/tardis/model/base.py 88.29% <0.00%> (ø)
tardis/tardis/plasma/properties/atomic.py 87.75% <0.00%> (ø)
tardis/tardis/plasma/properties/ion_population.py 85.71% <0.00%> (ø)
...dis/tardis/montecarlo/montecarlo_numba/r_packet.py 23.15% <0.00%> (ø)
...s/tardis/montecarlo/montecarlo_numba/macro_atom.py 43.75% <0.00%> (ø)
.../tardis/montecarlo/montecarlo_numba/interaction.py 24.32% <0.00%> (ø)
...dis/montecarlo/montecarlo_numba/numba_interface.py 48.48% <0.00%> (ø)
.../montecarlo/montecarlo_numba/single_packet_loop.py 24.56% <0.00%> (ø)

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 23ac153...0ea994d. Read the comment docs.

@KevinCawley
Copy link
Contributor Author

I do not know how to link this Issue: #1154

Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

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

@KevinCawley Nice work to investigate all of this! I've suggested few changes.

tardis/io/decay.py Outdated Show resolved Hide resolved
tardis/io/decay.py Outdated Show resolved Hide resolved
tardis/montecarlo/montecarlo_numba/interaction.py Outdated Show resolved Hide resolved
tardis/montecarlo/montecarlo_numba/single_packet_loop.py Outdated Show resolved Hide resolved
tardis/plasma/properties/atomic.py Outdated Show resolved Hide resolved
tardis/plasma/properties/atomic.py Outdated Show resolved Hide resolved
Changed the numba.array to numba.ndarray
Changed the r_packet type from RPacket to tardis.montecarlo.montecarlo_numba.r_packet.RPacket
Changed the Pandas DataFrame to pandas.DataFrame, and Numpy Array to numpy.ndarray. Made sure that the indentation was consistent as well.
Changed to Pandas DataFrame/Series/MultiIndex to be pandas.DataFrame/Series/MultiIndex. Also changed the numpy descriptor to numpy.ndarray, and for now moved the type as a comment.
tardis/montecarlo/montecarlo_numba/interaction.py Outdated Show resolved Hide resolved
tardis/plasma/properties/ion_population.py Outdated Show resolved Hide resolved
tardis/plasma/properties/ion_population.py Outdated Show resolved Hide resolved
tardis/montecarlo/montecarlo_numba/interaction.py Outdated Show resolved Hide resolved
tardis/montecarlo/montecarlo_numba/interaction.py Outdated Show resolved Hide resolved
tardis/montecarlo/montecarlo_numba/r_packet.py Outdated Show resolved Hide resolved
tardis/plasma/properties/atomic.py Outdated Show resolved Hide resolved
tardis/plasma/properties/atomic.py Outdated Show resolved Hide resolved
tardis/plasma/properties/ion_population.py Outdated Show resolved Hide resolved
KevinCawley and others added 12 commits December 17, 2020 13:09
Returns were mostly in : descriptor. Changed them to just be what type, which they are pandas.DataFrame as seen from the inheritance of the class IsotopeAbundances.
Removed trailing carriage returns on docstrings
Naming the variables by their full extensions, not just the class that they are.
Added the whole path (.NumbaPlasma) to the end of the description so the entire path is present.
Made the complete path for variables
Updated the descriptions to try and match comments. Fixed the descriptors for the numpy.ndarrays to make them in the same line.
Fixing the format of docstrings so that the dtype would be same line as the var : type
Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

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

Impressive work @KevinCawley - very minor changes and then we're good to go 🎉

tardis/plasma/properties/atomic.py Outdated Show resolved Hide resolved
tardis/montecarlo/montecarlo_numba/r_packet.py Outdated Show resolved Hide resolved
tardis/io/decay.py Show resolved Hide resolved
tardis/io/decay.py Show resolved Hide resolved
tardis/io/decay.py Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

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

Finally! 🚀

@jaladh-singhal jaladh-singhal changed the title Fix of Issue #1154 Fix incorrect docstrings Dec 18, 2020
@andrewfullard andrewfullard merged commit cadccff into tardis-sn:master Dec 18, 2020
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
* Fixed to the montecarlo_numba file to compy w/ issue tardis-sn#680

* Fixed the latex equation

* Trying to fix merge conflict

* revert to conf.py

* Remove extra carriage return

* Update tardis/io/decay.py

Co-authored-by: Jaladh Singhal <[email protected]>

* Update decay.py

* Update interaction.py

* Update tardis/plasma/properties/atomic.py

Co-authored-by: Jaladh Singhal <[email protected]>

* Update single_packet_loop.py

* Update atomic.py

Fixed the formatting in regards to @jalahdh-singhal 's comments

* Update numba_interface.py

Changed the numba.array to numba.ndarray

* Update r_packet.py

Changed the r_packet type from RPacket to tardis.montecarlo.montecarlo_numba.r_packet.RPacket

* Update ion_population.py

Changed the Pandas DataFrame to pandas.DataFrame, and Numpy Array to numpy.ndarray. Made sure that the indentation was consistent as well.

* Update atomic.py

Changed to Pandas DataFrame/Series/MultiIndex to be pandas.DataFrame/Series/MultiIndex. Also changed the numpy descriptor to numpy.ndarray, and for now moved the type as a comment.

* fixing merge conflicts

* Changed the returns to fit what type they are

Returns were mostly in : descriptor. Changed them to just be what type, which they are pandas.DataFrame as seen from the inheritance of the class IsotopeAbundances.

* Modification to docstrings and addressing comments

* Update base.py

Removed trailing carriage returns on docstrings

* Update interaction.py

Naming the variables by their full extensions, not just the class that they are.

* Update macro_atom.py

Added the whole path (.NumbaPlasma) to the end of the description so the entire path is present.

* Update r_packet.py

Made the complete path for variables

* Update atomic.py

Updated the descriptions to try and match comments. Fixed the descriptors for the numpy.ndarrays to make them in the same line.

* Update ion_population.py

Fixing the format of docstrings so that the dtype would be same line as the var : type

* Update docs/index.rst

Co-authored-by: Jaladh Singhal <[email protected]>

* Update tardis/plasma/properties/atomic.py

Co-authored-by: Jaladh Singhal <[email protected]>

* Update tardis/io/decay.py

Co-authored-by: Jaladh Singhal <[email protected]>

* Update decay.py

* Update r_packet.py

Co-authored-by: Jaladh Singhal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix docstrings being rendered incorrectly in API documentation
4 participants