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

DM-36616: Make a Getting Started Guide For Analysis Tools #79

Merged
merged 14 commits into from May 1, 2023

Conversation

sr525
Copy link
Collaborator

@sr525 sr525 commented Apr 17, 2023

No description provided.

Copy link
Member

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

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

Thanks for making the PR so it's easier to see and comment on the changes more easily. Overall I think this'll work. Having the example plots as static assets means there isn't a build-time requirement on making the plots, which simplifies this a lot to get going. Down the line we'll probably want to create the plots when the docs are built and I think the things we're working on to render notebooks on the RSP via an API could help a lot with this.

For consistency with all other Pipelines packages, the API reference section in index.rst is supposed to provide the full API for the module, including the [...]actions.plot, and [...]actions.scalar and [...]actions.vector sub-packages. Therefore, I think we should have automodapi listings for those packages in the API reference section of index.rst. Now I don't know if there'll be errors having two sets of automodapi statements for the same sub-packages, one for API reference and another for the bespoke action and plot listing pages, but I think it'd be worth trying. What I did for Pipeline tasks and configurations was to create a Sphinx extension that generated a custom listing of any subclass of lsst.pipe.base.Task or lsst.pex.config.Config. Should I extend that to also have custom listings for PlotAction, ScalarAction, and VectorAction types? Perhaps this Sphinx extension should show a thumbnail of the associated example plot.

Some recommendations on naming. The idiomatic naming for web page URLs is lowercase with - as a separator for words. The naming for rst pages follows that. So for example it's getting-started.rst rather than gettingStartedGuide.rst. You might also reconsider the naming of "current actions" and "current plots". The notion of the docs reflecting what's currently available is a given. So you might instead name them "Plot types" and "Plot actions".

Beyond that I see some potential syntax errors, but I don't know if you want to get into that right now. Let me know if you need help resolving them. But overall this should work for now.

@sr525
Copy link
Collaborator Author

sr525 commented Apr 20, 2023

For consistency with all other Pipelines packages, the API reference section in index.rst is supposed to provide the full API for the module, including the [...]actions.plot, and [...]actions.scalar and [...]actions.vector sub-packages. Therefore, I think we should have automodapi listings for those packages in the API reference section of index.rst. Now I don't know if there'll be errors having two sets of automodapi statements for the same sub-packages, one for API reference and another for the bespoke action and plot listing pages, but I think it'd be worth trying. What I did for Pipeline tasks and configurations was to create a Sphinx extension that generated a custom listing of any subclass of lsst.pipe.base.Task or lsst.pex.config.Config. Should I extend that to also have custom listings for PlotAction, ScalarAction, and VectorAction types? Perhaps this Sphinx extension should show a thumbnail of the associated example plot.

I have added the missing bits to the page, I've tried to include everything that I can on the index page. A thumbnail of the associated plots would be very cool.

@sr525
Copy link
Collaborator Author

sr525 commented Apr 20, 2023

Some recommendations on naming. The idiomatic naming for web page URLs is lowercase with - as a separator for words. The naming for rst pages follows that. So for example it's getting-started.rst rather than gettingStartedGuide.rst. You might also reconsider the naming of "current actions" and "current plots". The notion of the docs reflecting what's currently available is a given. So you might instead name them "Plot types" and "Plot actions".

I've updated everything to these naming conventions.

@sr525 sr525 force-pushed the tickets/DM-36616 branch 2 times, most recently from 3ad303d to ca6e2a1 Compare April 20, 2023 23:07
Copy link
Member

@jonathansick jonathansick left a comment

Choose a reason for hiding this comment

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

Overall the docs build, so we should go ahead with this. I've got some suggestions for cleaning up the warnings from the build.

I did a build with this branch and found a few warnings about docstrings that used Note as a section title instead of Notes. These should be straightforward to clean up:

Unknown section Note in the docstring of CalcE1 in python/lsst/analysis/tools/actions/vector/ellipticity.py.
Unknown section Note in the docstring of CalcE2 in python/lsst/analysis/tools/actions/vector/ellipticity.py.
Unknown section Note in the docstring of CalcShapeSize in python/lsst/analysis/tools/actions/vector/calcShapeSize.py.
Unknown section Note in the docstring of CalcE1 in python/lsst/analysis/tools/actions/vector/ellipticity.py.
Unknown section Note in the docstring of CalcE2 in python/lsst/analysis/tools/actions/vector/ellipticity.py.
Unknown section Note in the docstring of CalcShapeSize in python/lsst/analysis/tools/actions/vector/calcShapeSize.py.
Unknown section Note in the docstring of CalcE1 in python/lsst/analysis/tools/actions/vector/ellipticity.py.
Unknown section Note in the docstring of CalcE2 in python/lsst/analysis/tools/actions/vector/ellipticity.py.
Unknown section Note in the docstring of CalcShapeSize in python/lsst/analysis/tools/actions/vector/calcShapeSize.py.

I got some errors about lists in docstrings being poorly-formatted. These aren't part of the PR:

python/lsst/analysis/tools/actions/plot/histPlot.py:docstring of lsst.analysis.tools.actions.plot.histPlot.HistStatsPanel:9: WARNING: Bullet list ends without a blank line; unexpected unindent.
python/lsst/analysis/tools/actions/plot/histPlot.py:docstring of lsst.analysis.tools.actions.plot.histPlot.HistStatsPanel:12: WARNING: Bullet list ends without a blank line; unexpected unindent.

If you look at

I think the solution is to clean up the lists so that follow-on lines for each bullet item are consistently indented:

e.g.

    - The ListField parameter a dict to specify names of 3 stat columns accepts
      latex formating

    - The other parameters (stat1, stat2, stat3) are lists of strings that
      specify vector keys correspoinding to scalar values computed in the
      prep/process/produce steps of an analysis tools plot/metric configurable
      action. There should be one key for each group in the HistPanel.

The last category of errors is:

analysis_tools/python/lsst/analysis/tools/actions/vector/__init__.py:docstring of lsst.analysis.tools.actions.vector:1: WARNING: duplicate object description of lsst.analysis.tools.actions.vector, other instance in lsst.analysis.tools/action-types, use :noindex: for one of them
analysis_tools/python/lsst/analysis/tools/actions/scalar/__init__.py:docstring of lsst.analysis.tools.actions.scalar:1: WARNING: duplicate object description of lsst.analysis.tools.actions.scalar, other instance in lsst.analysis.tools/action-types, use :noindex: for one of them
analysis_tools/python/lsst/analysis/tools/actions/plot/__init__.py:docstring of lsst.analysis.tools.actions.plot:1: WARNING: duplicate object description of lsst.analysis.tools.actions.plot, other instance in lsst.analysis.tools/index, use :noindex: for one of them

I think the root of the problem is that modules are being documented in both plot-types.rst and index.rst. I think you might be able to solve this by adding :noindex: to the automodapi statements in plot-types.rst; I haven't tried this before though because duplicating automodapi statements isn't generally done.

Action Types
============

Here is a list of the current actions implemented in ``analysis tools``, please look at these carefully before
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Here is a list of the current actions implemented in ``analysis tools``, please look at these carefully before
Here is a list of the current actions implemented in ``analysis_tools``, please look at these carefully before

Comment on lines 15 to 20
:skip: ConfigurableActionField
:skip: ConfigurableActionStructField
:skip: DictField
:skip: Field
:skip: Vector
:skip: VectorAction
Copy link
Member

Choose a reason for hiding this comment

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

I think you can delete these as they're now generating a build warning indicating that these objects aren't available in the namespace.

My guess is that these objects are now dropped through a properly specified __all__ in the sub-modules that get import into lsst.analysis.tools.actions.vector.

Comment on lines 27 to 31
:skip: ChoiceField
:skip: Field
:skip: Scalar
:skip: ScalarAction
:skip: Vector
Copy link
Member

Choose a reason for hiding this comment

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

Same, I think you can delete these skips.

Comment on lines 23 to 26
.. .. toctree::
.. :maxdepth: 1
.. :glob:
.. currentActions
.. currentPlots
Copy link
Member

Choose a reason for hiding this comment

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

The toctree is being commented out. Re-enable it:

.. toctree:

   getting-started
   plot-types

Comment on lines 16 to 20
Using lsst.analysis.tools
=========================
For a tutorial on working with
``analysis_tools`` please see the :ref:`getting started guide <analysis-tools-getting-started>`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Using lsst.analysis.tools
=========================
For a tutorial on working with
``analysis_tools`` please see the :ref:`getting started guide <analysis-tools-getting-started>`.
Using lsst.analysis.tools
=========================
For a tutorial on working with
``analysis_tools`` please see the :ref:`getting started guide <analysis-tools-getting-started>`.

Plot Types
==========
Details of current plot types are here, these plots are all in ``python/lsst/analysis/tools/actions/plot`` and
can all be found on `github <https://github.com/lsst/analysis_tools/blob/main/python/lsst/analysis/tools/actions/plot/>`__.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
can all be found on `github <https://github.com/lsst/analysis_tools/blob/main/python/lsst/analysis/tools/actions/plot/>`__.
can all be found on `GitHub <https://github.com/lsst/analysis_tools/blob/main/python/lsst/analysis/tools/actions/plot/>`__.

Comment on lines 95 to 110
`"run"`
Output run for the plots (`str`).
`"tractTableType"`
Table from which results are taken (`str`).
`"plotName"`
Output plot name (`str`)
`"SN"`
The global signal-to-noise data threshold (`float`)
`"skymap"`
The type of skymap used for the data (`str`).
`"tract"`
The tract that the data comes from (`int`).
`"bands"`
The bands used for this data (`str` or `list`).
`"visit"`
The visit that the data comes from (`int`)
Copy link
Member

Choose a reason for hiding this comment

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

To get a dictionary list, start with a blank line then indent each description relative to its term:

Suggested change
`"run"`
Output run for the plots (`str`).
`"tractTableType"`
Table from which results are taken (`str`).
`"plotName"`
Output plot name (`str`)
`"SN"`
The global signal-to-noise data threshold (`float`)
`"skymap"`
The type of skymap used for the data (`str`).
`"tract"`
The tract that the data comes from (`int`).
`"bands"`
The bands used for this data (`str` or `list`).
`"visit"`
The visit that the data comes from (`int`)
`"run"`
Output run for the plots (`str`).
`"tractTableType"`
Table from which results are taken (`str`).
`"plotName"`
Output plot name (`str`)
`"SN"`
The global signal-to-noise data threshold (`float`)
`"skymap"`
The type of skymap used for the data (`str`).
`"tract"`
The tract that the data comes from (`int`).
`"bands"`
The bands used for this data (`str` or `list`).
`"visit"`
The visit that the data comes from (`int`)

Comment on lines 255 to 258
`"run"`
Output run for the plots (`str`).
`"tableName"`
Name of the table from which results are taken (`str`).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`"run"`
Output run for the plots (`str`).
`"tableName"`
Name of the table from which results are taken (`str`).
`"run"`
Output run for the plots (`str`).
`"tableName"`
Name of the table from which results are taken (`str`).

Comment on lines 153 to 160
``"run"``
The output run for the plots (`str`).
``"skymap"``
The type of skymap used for the data (`str`).
``"filter"``
The filter used for this data (`str`).
``"tract"``
The tract that the data comes from (`str`).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
``"run"``
The output run for the plots (`str`).
``"skymap"``
The type of skymap used for the data (`str`).
``"filter"``
The filter used for this data (`str`).
``"tract"``
The tract that the data comes from (`str`).
``"run"``
The output run for the plots (`str`).
``"skymap"``
The type of skymap used for the data (`str`).
``"filter"``
The filter used for this data (`str`).
``"tract"``
The tract that the data comes from (`str`).

Comment on lines +7 to +11
.. automodapi:: lsst.analysis.tools.actions.plot
:no-inherited-members:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. automodapi:: lsst.analysis.tools.actions.plot
:no-inherited-members:
.. automodapi:: lsst.analysis.tools.actions.plot
:no-inherited-members:
:noindex:

This this; I'm not certain it'll work but might solve the warnings about duplicate documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't solve the warnings and it breaks things elsewhere. I'm not sure what to do about this, but if it won't break anything I will merge it and fix it later.

@sr525
Copy link
Collaborator Author

sr525 commented Apr 28, 2023

I think the root of the problem is that modules are being documented in both plot-types.rst and index.rst. I think you might be able to solve this by adding :noindex: to the automodapi statements in plot-types.rst; I haven't tried this before though because duplicating automodapi statements isn't generally done.

I have tried this and it didn't work. It got rid of all of the links in both places.

@jonathansick
Copy link
Member

Ok, sounds like we'll have to live with the warnings for now. This would be solved with a custom class listing I mentioned before that I'm adding to my to-do list.

@sr525 sr525 merged commit f65ce6d into main May 1, 2023
7 checks passed
@sr525 sr525 deleted the tickets/DM-36616 branch May 1, 2023 16:09
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.

None yet

2 participants