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

DOCS: improved documentation #155

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Conversation

antoinecollet5
Copy link
Contributor

@antoinecollet5 antoinecollet5 commented Oct 4, 2023

Several changes I propose for the documentation:

Documentation:

  • Use the modern pydata template rather than rtd (used by mainstream packages such as numpy and scipy)
  • Include the readme in the doc for a nice rendering and avoid duplication
  • Make links between the bibliography and the docstrings using the :cite: directive.
  • Improved docstrings for SIES and ES (differentiate class attributes for method parameters, detail inversion types) .
  • Better API section
  • Add makefiles for documentation build (both unix and windows) because tox does not work offline.
  • Add badges in readme for better visibility

@tommyod
Copy link
Collaborator

tommyod commented Oct 4, 2023

Will look over this more closely soon. :) We just merged a big overhaul of SIES, leading to some conflicts unfortunately. You can probably just merge main and checkout ``--theirs` to solve the conflicts without too much trouble.

@tommyod
Copy link
Collaborator

tommyod commented Oct 6, 2023

Hi. Fantastic work as always @antoinecollet5 ! Mostly looks good to me. A few comments:

  • You'll have to merge in our recent changes unfortunately.
  • We want to avoid makefiles. Can you elaborate on why they are needed if they are? If they are not needed, then we should remove them from the PR.

I'll also try to run it locally before we merge, just to see what the docs look like. But I'll wait until you have merged main :) Good stuff. Thanks for the effort. Greatly appreciate it.

@antoinecollet5 antoinecollet5 force-pushed the feature/docs branch 3 times, most recently from 6930cd6 to 8881033 Compare October 6, 2023 13:43
@antoinecollet5
Copy link
Contributor Author

antoinecollet5 commented Oct 6, 2023

@tommyod thanks for the feedbacks :) I found a bit of time to solve the conflicts and adapt to the new interface. Note that I removed the example in the readme since it is now outdated.

Regarding the Makefiles, I just removed them. However, I noticed that the html rendering is a bit different than when using tox. I don't get why yet....

Wait for your feedback about the html rendering. Note that I did not take care of the pdf export.

Copy link
Collaborator

@tommyod tommyod left a comment

Choose a reason for hiding this comment

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

Overall very excited about this. New theme looks great. Many improvements here. I do have some comments however :)

New theme looks good:

image

But LaTeX does not render:

image

Adding "sphinx.ext.mathjax" to the extensions in conf.py did not help me locally.

  • Could you take a look at LaTeX? Does it work for you?
  • I added some comments along the way, please take a look.
  • In the future, even smaller PRs is good. For instance, I am not so sure about documenting attributes, but willing to discuss it. However, it's not really related to the issue of updating sphinx etc - so it would've been even better to have a PR for documentation changes (unless they are strictly required to make sphinx run).

docs/source/_templates/autosummary/module.rst Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/index.rst Show resolved Hide resolved
src/iterative_ensemble_smoother/sies.py Outdated Show resolved Hide resolved
@antoinecollet5 antoinecollet5 force-pushed the feature/docs branch 2 times, most recently from 35730bf to 29c4a35 Compare October 19, 2023 21:57
@antoinecollet5
Copy link
Contributor Author

@tommyod I had a quick look at latex and indeed it does not render. I propose you to see that in another issue/PR ? And to discuss whether the latexpdf export is really needed ?

@tommyod
Copy link
Collaborator

tommyod commented Oct 20, 2023

And to discuss whether the latexpdf export is really needed ?

Sorry, I was unclear on this. Let me try again: the latexpdf is not needed, but math should render in the HTML. For me it does not render correctly in HTML (see screenshot above, same now as before). Does it render in the HTML docs for you?

I see that m2r2 expects inline math as `$e=mc^2$`, with the backticks. We use $e=mc^2$ in our markdown. For blocks of math we use $$e=mc^2$$ in the notebooks. I am not sure what m2r2 excepts here, or if it supports it at all.

I propose you to see that in another issue/PR ?

I know it's tedious, but I'd rather get HTML math to work in this PR. I don't want to merge PRs that improve some things, but regress on others, and think that we'll solve it in a future PR. This kind of thinking has a tendency to lead to messy code, especially when it's not obvious what the fix is, who will dedicate time to fix it, etc.

@antoinecollet5
Copy link
Contributor Author

antoinecollet5 commented Oct 20, 2023

@tommyod normally, math should now render correctly in the html export. Have you tried since yesterday's changes ?

This is what I get now:

image

@tommyod
Copy link
Collaborator

tommyod commented Oct 20, 2023

I probably did something wrong. Let me try again and report back ... :)

…kefiles for unix and windows to avoid having to use tox, use pydata theme rather than read the docs and improve docstrings for classes SIES and ES. Add badges for project visibility.
@tommyod
Copy link
Collaborator

tommyod commented Oct 23, 2023

Fresh try:

git remote add antoine https://github.com/antoinecollet5/iterative_ensemble_smoother.git
git fetch --all
git switch feature/docs
pip install -e '.[dev,doc]'
python -m ipykernel install --user
sphinx-build -c docs/source/ -b html docs/source/ docs/build/html -W
firefox docs/build/html/index.html

And it works and looks great. 👍

Sorry for the hiccup and thanks for the patience. I must have done something wrong the last time I tried.

Some images of the new wonderful docs:

image

image

image

image

Copy link
Collaborator

@tommyod tommyod left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again for the work @antoinecollet5 . Very much appreciated as always!

@tommyod tommyod merged commit dbbb641 into equinor:main Oct 23, 2023
9 checks passed
@antoinecollet5 antoinecollet5 deleted the feature/docs branch October 23, 2023 15:18
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.

2 participants