Skip to content

Fix Sphinx>4 warnings with units and coordinates#13048

Closed
saimn wants to merge 2 commits intoastropy:mainfrom
saimn:sphinx4
Closed

Fix Sphinx>4 warnings with units and coordinates#13048
saimn wants to merge 2 commits intoastropy:mainfrom
saimn:sphinx4

Conversation

@saimn
Copy link
Contributor

@saimn saimn commented Apr 1, 2022

Another attempt to fix #11725.

Description

This pull request is to address ...

Fixes #

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2022

You will be upgraded.

(A special day message.)

@pllim
Copy link
Member

pllim commented Apr 1, 2022

Please don't tell me this is April Fool PR. My heart can't take it. It looks so elegant!

@pllim
Copy link
Member

pllim commented Apr 1, 2022

Don't worry about mpl or numpy dev failures. They are not related.

@pllim
Copy link
Member

pllim commented Apr 1, 2022

RTD is green! 🎉

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

The diff LGTM but coordinates maintainers (particularly @adrn ?) should see if the rendered API doc is still ok.

@pllim pllim requested review from adrn and larrybradley April 1, 2022 21:13
@pllim
Copy link
Member

pllim commented Apr 1, 2022

cc @maxnoe

@maxnoe
Copy link
Member

maxnoe commented Apr 2, 2022

But that just removes the corresponding documentation. This is not what we want, right? We want a way to avoid these warnings without loosing the documentation pages.

@saimn
Copy link
Contributor Author

saimn commented Apr 2, 2022

Yes, so the idea was first to see if this fixes the warnings. Those functions are already documented at the top level, and are duplicated in a specific section.
For coordinates documenting the built-in frames in a specific section makes a lot of sense but I don't think there is way currently to remove this module from the top-level documentation (to avoid the duplicate warning). I think it's possible to skip a class/function but not a whole module ? Maybe something we can improve in sphinx-automodapi.
For units it's less clear to me, there are other modules that are duplicated but do not raise the warning ...

@pllim
Copy link
Member

pllim commented Apr 4, 2022

IMHO the benefit of getting rid of two maxversion pins outweighs the cost of losing some duplicate doc in coordinates. I have no idea when sphinx-automodapi will get fixed and this has been plaguing us long enough.

Can we go ahead with this and then open up follow-up issue (or comment on existing ones)?


.. automodapi:: astropy.units.equivalencies

.. automodapi:: astropy.units.function
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these removed? They are distinct from astropy.units.function.units and, e.g., STmag does not seem to be linked anywhere else on the present doc pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I had to remove this one as well to fix the warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it works with it 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

And how about function.logarithmic? Is removing that truly needed? If present, what is the error?

Built-in Frame Classes
======================

.. automodapi:: astropy.coordinates.builtin_frames
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess a similar question here - certainly, with this removed, the section title should be removed too -- but this particular line is a very big deal in the actual docs!! https://docs.astropy.org/en/latest/coordinates/index.html#built-in-frame-classes

@pllim
Copy link
Member

pllim commented Apr 7, 2022

As discussed at infrastructure tag-up today (2022-04-07) between @saimn , @eteq , @bsipocz , @tomdonaldson , and myself, here is the plan:

  1. @eteq will look into a proper upstream fix for Incompatible with sphinx>4 (submodules of submodules) sphinx-automodapi#130 (some useful discussion in that issue including https://github.com/maxnoe/sphinx-automodapi-duplicated-warning to reproduce this outside of astropy) or possibility of Can this package be replaced by something else already in Sphinx? sphinx-automodapi#147
  2. If proper fix not implemented by v5.2 feature freeze time (end of 2022), we clean up and merge PR at the cost of some documentation loss.

Stretch goal:

@pllim pllim modified the milestones: v5.0.5, v5.2 Apr 7, 2022
@saimn
Copy link
Contributor Author

saimn commented Apr 19, 2022

See sphinx-doc/sphinx#10348 (comment) for a great description of the problem and the different options to solve this. Closing, we should continue the discussion in astropy/sphinx-automodapi#130.

@saimn saimn closed this Apr 19, 2022
@saimn saimn deleted the sphinx4 branch April 19, 2022 19:37
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.

DOC: Unpin max version of Sphinx when Sphinx 4.0.2 is out (and Jinja2)

4 participants