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

Fix some doc warnings and convert to myst #482

Merged
merged 10 commits into from
Oct 14, 2021

Conversation

ascillitoe
Copy link
Contributor

@ascillitoe ascillitoe commented Sep 20, 2021

This PR converts the alibi docs to use myst files instead of rst (for consistency with alibi-detect SeldonIO/alibi-detect#325).

This allows for the now deprecated m2r to be removed, and the use of newer sphinx versions going forwards.

TODO:

  • A thorough check of the compiled html and latex docs, including api docs. - All looks OK, except for known issue with Bases in api docs.
  • Update doc/README.md with a few myst specific details. - Done
  • Include xgboost_model_fitting_adult.ipynb in examples, or add orphan tag. - Orphan tag added.

Relevant issues:

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Sep 20, 2021

Codecov Report

Merging #482 (965d92c) into master (ef757b9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #482   +/-   ##
=======================================
  Coverage   82.32%   82.32%           
=======================================
  Files          76       76           
  Lines       10315    10315           
=======================================
  Hits         8492     8492           
  Misses       1823     1823           
Impacted Files Coverage Δ
alibi/explainers/anchor_text.py 86.61% <ø> (ø)
alibi/utils/lang_model.py 96.58% <ø> (ø)

@ascillitoe ascillitoe requested review from jklaise and removed request for jklaise September 27, 2021 11:20
@ascillitoe
Copy link
Contributor Author

More detailed checks picked up a number of issues in the docs with some (especially amsmath) math environments within Jupyter notebook-derived pages. Standard display math i.e. $$...$$ is fine, but math blocks such as:

\begin{equation}\tag{1}
...
\end{equation}

and

\begin{align}

\end{align}

are not showing up properly in html. alibi-detect is fine for now as we don't have such complex math use cases in the docs. This issue seems to be something to do with myst overriding the mathjax config. Setting myst_update_mathjax=False in conf.py helps sometimes, but not reliably.

These issues seem to all be fixed in sphinx>=4.0 so I think we should wait until we upgrade to sphinx==4.3.0 before moving the alibi docs to myst (related to SeldonIO/alibi-detect#338).

@jklaise jklaise mentioned this pull request Oct 11, 2021
@ascillitoe
Copy link
Contributor Author

Update: After offline discussion, we have decided to go ahead and merge this in before sphinx 4.3.0 is released. This will break the Bases field in api docs for submodules/subpackages such as https://docs.seldon.io/projects/alibi/en/latest/api/alibi.models.tensorflow.actor_critic.html.

Since this PR will now be merged in, I have opened an issue (#505) as a reminder to update.

requirements/docs.txt Outdated Show resolved Hide resolved
requirements/docs.txt Outdated Show resolved Hide resolved
requirements/docs.txt Outdated Show resolved Hide resolved
requirements/docs.txt Outdated Show resolved Hide resolved
requirements/docs.txt Outdated Show resolved Hide resolved
@ascillitoe ascillitoe merged commit 5768a26 into SeldonIO:master Oct 14, 2021
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