Hello @nstarman 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-04-30 15:16:26 UTC

@nstarman - I haven't looked in detail yet, but about storage I wonder whether you considered repurposing the way units are represented in the docs, which is basically by automatically generating the docstring for each module (see generate_units_summary in utils.py). I think I'd prefer that over a .csv file.

Regardless, maybe best to do that kind of docs change as a separate PR from using the physical types in the docs?

On that: would #10655 enable moving towards allowing ~astropy.units.Quantity['length']?

#not_v4.3

See #11618 for just getting the physical types as a table in the docs.

Great. I'll label this as requiring 11618, and wait until that is merged. This PR will expand upon 11618 to add ReST link targets in the table.

Ok. Trying to figure out how to get rid of that ugly spare column in the physical types table, but the link targets are up and running!
Now for adding docstring references all over the place.

This PR looks great! I can't wait for us to be able to do the same thing for PlasmaPy.

@namurphy your comment makes me wonder: @nstarman are the xref physical types here usable by affiliated packages through intersphinx?

Would be good to remember that we should probably document this PR in two places: (1) in the documentation part of the contributing guide (here?) to remind documentation contributions to include physical types when quantities/units are involved, and (2) to guide affiliated package maintainers with how to use xref physical types in their own docs, either through intersphinx or by copy-pasting the relevant part of the conf.py file modified here. Make a new issue to keep track of these things (for a follow-up)?

@namurphy your comment makes me wonder: @nstarman are the xref physical types here usable by affiliated packages through intersphinx?

That's a good question. They should be according to https://docs.readthedocs.io/en/stable/guides/intersphinx.html. One of my goals is to improve the overall ease of intersphinxing with astropy and using the glossary and xref aliases. Part of that goal is to move some of this and other code to sphinx_astropy, and pre-set up the inter sphinx... but until then, something like the following might work for numpydoc_xref in affiliate packages:

1. add astropy to intersphinx
2. map in your conf.py

from astropy.units.physical import _units_and_physical_types
numpydoc_xref_physical_type_aliases = {}
for _, ptypes in _units_and_physical_types:
    ptypes = {ptypes} if isinstance(ptypes, str) else ptypes
    for ptype in ptypes:
        key = f"'{ptype}'"
        val = f":ref:`:ref: '{ptype}' <astropy:{ptype}>`"   # <== intersphinxed link here
        numpydoc_xref_physical_type_aliases[key] = val

numpydoc_xref_aliases.update(numpydoc_xref_physical_type_aliases)

Would be good to remember that we should probably document this PR in two places: (1) in the documentation part of the contributing guide (here?) to remind documentation contributions to include physical types when quantities/units are involved, and (2) to guide affiliated package maintainers with how to use xref physical types in their own docs, either through intersphinx or by copy-pasting the relevant part of the conf.py file modified here. Make a new issue to keep track of these things (for a follow-up)?

👍 #11675

@nstarman — thank you! I've got too much pandemic brain fog to think much about it at the moment, but that looks like a good way to go about it for the time being. I'm hoping to look into this more after 4.3 is released.

A slight hesitation is that _units_and_physical_types is not part of Astropy's public API, so I'm wondering if it would be worthwhile to put a comment in physical.py that says not to change the name of that dict since it may end up being used in other packages for this purpose.

cc: @rocco8773

A slight hesitation is that _units_and_physical_types is not part of Astropy's public API, so I'm wondering if it would be worthwhile to put a comment in physical.py that says not to change the name of that dict since it may end up being used in other packages for this purpose.

@namurphy, my thought for other packages is to essentially move the short script (if @adrn or others confirm it works) in #11595 (comment) to sphinx_astropy and export numpydoc_xref_physical_type_aliases (perhaps renamed to include "astropy") into v1 so that in 3rd party packages' conf.py it gets imported. Then anyone who wants the physical-type xrefs can do numpydoc_xref_aliases.update(numpydoc_xref_physical_type_aliases). If they wanted different names, e.g. ['astropy-length'] instead of ['length'] if they support more than one unit package, then that can be done by
numpydoc_xref_aliases.update({"'astropy-"+k[1:]: v for k, v in numpydoc_xref_physical_type_aliases.items()}).
This would hide _units_and_physical_types and make xref plugin quite easy. I think implementing #11595 (comment) directly in 3rd party packages should remain an experimental implementation and the next release of sphinx_astropy should codify the implementation for 3rd party packages.

Alternatively/additionally, a good addition to physical.py might be to add each physical type to the module (like how cosmology realizations are added in cosmology/realizations.py) and have a set of all their names.
available_physical_types= _name_physical_mapping.keys() would conveniently stay updated as people define new physical types. Were this implemented, then that would change #11595 (comment) and this PR to use all publicly-scoped functions.

astropy/sphinx-astropy#39

@namurphy I think using _units_and_physical_types is a temporary solution: we would ideally make it easier for other packages to get access to the physical type definitions directly in the future. So maybe better to put the comment in your docs :)

Edit: As I clicked comment here, I saw @nstarman commented above - yes to what he says!

I think this is ready. Thanks again @nstarman - this is a great addition to the documentation!