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

Review all doc pages before tagging v3 #1116

Merged
merged 37 commits into from
Aug 8, 2023
Merged

Review all doc pages before tagging v3 #1116

merged 37 commits into from
Aug 8, 2023

Conversation

damonge
Copy link
Collaborator

@damonge damonge commented Jul 28, 2023

We need to review all of the doc pages on readthedocs, since some of them are a bit antiquated, and some of the autodoc pages show small typos/inaccuracies. This is the last thing to do before we're able to tag v3!

A few instructions for reviewers:

  • Virtually no code has changed. I've marked below in comments the only places where this has happened, which had to do with things that had been deprecated long ago but which I only noticed while going through the documentation. Besides this, I don't think it's worth looking at the code itself.
  • The most efficient way to review this PR would be to have a systematic look at the documentation pages. I've activated them for this branch here: https://ccl.readthedocs.io/en/doc_review/. I would thus suggest reviewers to look at the doc pages and flag issues as comments here, rather than looking at the code itself.

Of course, reviewers should feel free to proceed as they prefer!

Closes #1091
Closes #867
Closes #871

@coveralls
Copy link

coveralls commented Jul 28, 2023

Pull Request Test Coverage Report for Build 5789866361

  • 6 of 6 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.448%

Totals Coverage Status
Change from base Build 5744252840: 0.0%
Covered Lines: 5805
Relevant Lines: 5957

💛 - Coveralls

@hsinfan1996
Copy link
Contributor

Update this line?

copyright = u'2018-2019, LSST DESC'

@hsinfan1996
Copy link
Contributor

The link is outdated. Should be https://www.numpy.org/

'numpy': ('http://docs.scipy.org/doc/numpy', None),

@damonge damonge marked this pull request as ready for review August 1, 2023 09:30
@hsinfan1996
Copy link
Contributor

A more detailed table of contents for API Documentation would be great, e.g.
image

@hsinfan1996
Copy link
Contributor

Is there a reason to use this specific version of sphinx_rtd_theme?

sphinx_rtd_theme==1.2.0rc3

@chrgeorgiou chrgeorgiou self-requested a review August 1, 2023 13:44
@damonge
Copy link
Collaborator Author

damonge commented Aug 1, 2023

@hsinfan1996 a few months ago the default rtd theme had a few bugs that affected how some things were displayed. We could check if this is now fixed.

Regarding the TOC: do you have a conf.py showing how to do this somewhere?

@hsinfan1996
Copy link
Contributor

@hsinfan1996 a few months ago the default rtd theme had a few bugs that affected how some things were displayed. We could check if this is now fixed.

Regarding the TOC: do you have a conf.py showing how to do this somewhere?

conf.py from CLMM
https://github.com/LSSTDESC/CLMM/blob/c6cf806cd9b542f7286187ed0ecb6432f959a767/docs/conf.py#L216-L229

@hsinfan1996
Copy link
Contributor

hsinfan1996 commented Aug 1, 2023

@hsinfan1996 a few months ago the default rtd theme had a few bugs that affected how some things were displayed. We could check if this is now fixed.
Regarding the TOC: do you have a conf.py showing how to do this somewhere?

conf.py from CLMM https://github.com/LSSTDESC/CLMM/blob/c6cf806cd9b542f7286187ed0ecb6432f959a767/docs/conf.py#L216-L229

@damonge Turns out it is just a html_theme_options to be added in the conf.py. No need to modify any .rst files.

@anicola
Copy link
Contributor

anicola commented Aug 1, 2023

I have started going through the top-level docs and just found these super small things. I will now start going through the function docs.
image
image
image

readthedocs/conf.py Outdated Show resolved Hide resolved
@hsinfan1996
Copy link
Contributor

hsinfan1996 commented Aug 2, 2023

I removed modules.rst so the TOC looks like
image
instead of
image

I can revert it if anyone wants pyccl package to be at the first level.

@damonge
Copy link
Collaborator Author

damonge commented Aug 2, 2023

This looks good. Thanks @hsinfan1996 !

@damonge
Copy link
Collaborator Author

damonge commented Aug 2, 2023

Thanks a lot @anicola , super useful! Just fixed those.

@damonge
Copy link
Collaborator Author

damonge commented Aug 8, 2023

This has been reviewed now by several people, so I'm just using my super powers to merge.

@damonge damonge merged commit 29d4697 into master Aug 8, 2023
4 checks passed
@damonge damonge deleted the doc_review branch August 8, 2023 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants