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] Change docs theme to pydata-sphinx theme #4985

Merged
merged 8 commits into from
Nov 17, 2022

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Nov 9, 2022

This PR:

  • Switches the python docs theme to pydata-sphinx-theme to be uniform with rest of the rapids python docs.
  • Fixes typos
  • Fixes a number of formatting and alignment issues in docstrings.
  • Discuss with cuml team on how(if any) and where things should be placed.

@galipremsagar galipremsagar requested review from a team as code owners November 9, 2022 03:07
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added conda conda issue CUDA/C++ Cython / Python Cython or Python issue labels Nov 9, 2022
@caryr35 caryr35 added this to PR-WIP in v22.12 Release via automation Nov 14, 2022
@caryr35 caryr35 moved this from PR-WIP to PR-Needs review in v22.12 Release Nov 14, 2022
@galipremsagar galipremsagar added doc Documentation breaking Breaking change labels Nov 15, 2022
@galipremsagar galipremsagar changed the title [WIP] Change docs theme to pydata-sphinx theme [REVIEW] Change docs theme to pydata-sphinx theme Nov 15, 2022
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

I ran this locally and it really looks great to me overall. I like that the GitHub and RAPIDS twitter links are right on the top and I was having fun playing with the light/dark color modes. The code all appears to display nicely and the source links work.

The only thing I would change is to shorten the top-level section headings just a tad. Small things like removing "cuml" (since cuml is already displayed on the left and users already know the docs are for cuml) from section headings on the top could help make things easier to consume.

We can discuss that stuff further, though, and open follow-up PRs if we feel it should be changed.

v22.12 Release automation moved this from PR-Needs review to PR-Reviewer approved Nov 16, 2022
@beckernick
Copy link
Member

beckernick commented Nov 16, 2022

Do we need this file anymore? https://github.com/rapidsai/cuml/blob/branch-22.12/conda/environments/builddocs_py36.yml

[Can discuss in follow ups]

@galipremsagar
Copy link
Contributor Author

Do we need this file anymore? https://github.com/rapidsai/cuml/blob/branch-22.12/conda/environments/builddocs_py36.yml

[Can discuss in follow ups]

Not at all, I've used [cuml_dev_cuda11.5.yml] to build and deploy the docs. Will remove it as part of this PR.

@galipremsagar
Copy link
Contributor Author

I ran this locally and it really looks great to me overall. I like that the GitHub and RAPIDS twitter links are right on the top and I was having fun playing with the light/dark color modes. The code all appears to display nicely and the source links work.

The only thing I would change is to shorten the top-level section headings just a tad. Small things like removing "cuml" (since cuml is already displayed on the left and users already know the docs are for cuml) from section headings on the top could help make things easier to consume.

We discuss that stuff further, though, and open follow-up PRs if we feel it should be changed.

Yup, can shorten those. Will do it here.

Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

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

@galipremsagar just holding this PR open while you make the changes so it doesn't accidentally get merged.

v22.12 Release automation moved this from PR-Reviewer approved to PR-Needs review Nov 16, 2022
@galipremsagar galipremsagar added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Nov 16, 2022
@galipremsagar
Copy link
Contributor Author

galipremsagar commented Nov 16, 2022

@cjnolet @beckernick Addressed your reviews and this should be ready for another look. The top bar will now look much cleaner, let me know what you think.
Screen Shot 2022-11-16 at 4 59 04 PM

@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels Nov 16, 2022
@galipremsagar
Copy link
Contributor Author

rerun tests

v22.12 Release automation moved this from PR-Needs review to PR-Reviewer approved Nov 17, 2022
@cjnolet
Copy link
Member

cjnolet commented Nov 17, 2022

@gpucibot merge

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Nov 17, 2022
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.12@0f19b92). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.12    #4985   +/-   ##
===============================================
  Coverage                ?   79.60%           
===============================================
  Files                   ?      184           
  Lines                   ?    11698           
  Branches                ?        0           
===============================================
  Hits                    ?     9312           
  Misses                  ?     2386           
  Partials                ?        0           
Flag Coverage Δ
dask 45.93% <0.00%> (?)
non-dask 69.14% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rapids-bot rapids-bot bot merged commit dd197b9 into rapidsai:branch-22.12 Nov 17, 2022
v22.12 Release automation moved this from PR-Reviewer approved to Done Nov 17, 2022
rapids-bot bot pushed a commit to rapidsai/raft that referenced this pull request Nov 18, 2022
Similar to rapidsai/cuml#4985, this PR changes the docs theme for `raft` to be in-line with rest of the rapids docs theme.

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: #1026
jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
This PR:

- [x] Switches the python docs theme to `pydata-sphinx-theme` to be uniform with rest of the `rapids` python docs.
- [x] Fixes typos
- [x] Fixes a number of formatting and alignment issues in docstrings.
- [x] Discuss with cuml team on how(if any) and where things should be placed.

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - AJ Schmidt (https://github.com/ajschmidt8)

URL: rapidsai#4985
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change conda conda issue CUDA/C++ Cython / Python Cython or Python issue doc Documentation
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants