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

Cleanup after second pass through stellarphot.io documentation. #100

Merged
merged 17 commits into from
Jun 16, 2023

Conversation

JuanCab
Copy link
Contributor

@JuanCab JuanCab commented Jun 15, 2023

This is likely another "epic" pull request. After reviewing some of the class UMLs I was generating from the stellarphot code, I realized there were a litany of other issues with the docstrings that needed to be dealt with. Among these issues:

  • More consistent formatting of types that link to documentation.
  • Fixing rendering of equations from docstrings in sphinx.
  • Removing documentation of class methods from main class docstring (its redundant).
  • Adding documentation to __init__.py initialization method for all classes, since it is a public method.

In the process of performing this second review of the docstrings I found a few public methods with no docstring and tried to address that as well.

Copy link
Contributor

@mwcraig mwcraig 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. I think we only want the leading ~ when referring to stellarphot stuff. Looking at the numpydoc documentation for the first time in a long time it looks like we can maybe not use backticks around the arguments? That seems like it is maybe the case in their example. Probably not important though.

I stopped removing the ~ about halfway through. If you go the route of accepting suggestions, it is much easier to add them to a batch than to add them one by one.

@@ -99,7 +99,7 @@ def make_checker(indicator_widget, value_widget):
Parameters
----------

indicator_widget : MyCheck widget
indicator_widget : `~stellarphot.analysis.MyValid` widget
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure we need the full package name in this case because the class is in the package. I.e. MyValid might work just fine.

Reading through https://numpydoc.readthedocs.io/en/latest/index.html now to try to see....and after reading the (genuinely useful) documentation I'm still not sure.

stellarphot/analysis/exotic.py Show resolved Hide resolved
stellarphot/analysis/transit_fitting.py Show resolved Hide resolved
stellarphot/analysis/transit_fitting.py Show resolved Hide resolved
stellarphot/core.py Show resolved Hide resolved
sources
an astropy table of the positions of sources in the image.
If `find_fwhm` is ``True``, includes a column called ``FWHM``.
sources: `~astropy.table.Table`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sources: `~astropy.table.Table`
sources: `astropy.table.Table`

@@ -44,7 +44,7 @@ def seeing_plot(raw_radius, raw_counts, binned_radius, binned_counts, HWHM,
Returns
-------

`matplotlib.pyplot.figure`
`~matplotlib.pyplot.figure`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`~matplotlib.pyplot.figure`
`matplotlib.pyplot.figure`

@@ -46,9 +46,9 @@ def read_file(radec_file):
Returns
-------

`astropy.table.Table`
`~astropy.table.Table`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`~astropy.table.Table`
`astropy.table.Table`

Table with target information, including a
`astropy.coordinates.SkyCoord` column.
`~astropy.coordinates.SkyCoord` column.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`~astropy.coordinates.SkyCoord` column.
`astropy.coordinates.SkyCoord` column.

Copy link
Contributor Author

@JuanCab JuanCab left a comment

Choose a reason for hiding this comment

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

I've provided my feedback to you.

stellarphot/core.py Show resolved Hide resolved
stellarphot/differential_photometry/aij_rel_fluxes.py Outdated Show resolved Hide resolved
stellarphot/io/tess.py Show resolved Hide resolved
Copy link
Contributor

@mwcraig mwcraig left a comment

Choose a reason for hiding this comment

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

Down to just a couple really minor things. Really not sure what happened with that batch 🤷‍♂️

stellarphot/core.py Show resolved Hide resolved
stellarphot/io/aij.py Outdated Show resolved Hide resolved
stellarphot/io/tess.py Show resolved Hide resolved
@JuanCab
Copy link
Contributor Author

JuanCab commented Jun 16, 2023

I did push through a big set of edits removing the tildas from all the non stellarphot packages. That hopefully handled most of the issues. Will finish the rest next week.

@mwcraig mwcraig merged commit e7093ee into feder-observatory:main Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants