Closed Fanchengyan closed 3 months ago
Hey, thank you very much for your contribution to EOmaps! Highly appreciated!
I've already been thinking about moving to the PyData theme as well for quite some time now but never managed to find the time to actually give it a try... really curious to see how the docs look with this theme! Screenshots already look really nice!
Some initial comments from my side before I can review your changes:
I see that you have commited all the files from the API docs (e.g. files in the "generated" folder) which is why there are now 647 changed files... Those files are auto-generated by sphinx-autodoc
on each build and should not be part of the source-code (they would be overwritten during the build-process anyways).
Could you please remove those files from your PR? It would make it a lot easier to review your actual changes.
(The top-level .gitighore
file should actually avoid that files in the generated
folder are commited... )
Also... what is the purpose of this GitButtler Integration Commit
?
Tests are currently failing due to code-style issues reported by pre-commit.
Can you please run pre-commit to auto-format your changes so that they comply with the used code-style?
You can find instructions on how to use pre-commit
in the docs: contribute - style-cecking. Thanks!
OK, I'll revise it again soon.
It should be fine now. The documentation can be previewed at: https://eomaps--249.org.readthedocs.build/en/249/index.html
Awesome! It looks amazing 🤩
However, the "docs/generated"
folder is still part of the PR so there are still 649 changed files... can you please remove it so that the auto-generated API docs are not part of the repo? Thanks!
Once this is done I'll try to find some time as soon as possible to do a proper review of the changes to see if all looks (and works) as expected.
Again, thanks a lot for your efforts here!
I added a commit that deleted the generated folder. The compilation of the document itself shouldn't be a major issue, but there are two problems that may need further resolution:
I've only done an initial restructuring of the document, and it may need further additions/modifications to some content. For example, some newly added items in the header don't have emojis, and some section index.rst files may need additional content to guide users into specific subsections.
Because the structure of the document has changed, the content in the test section may need to be updated accordingly.
Hi @raphaelquast
I've modified the pytest code related to the docs section, and it's now running properly. There should be no issues now. This is my first pull request, so the number of iterations might be a bit high. If there are any other problems, please let me know.
I've only done an initial restructuring of the document, and it may need further additions/modifications to some content. For example, some newly added items in the header don't have emojis, and some section index.rst files may need additional content to guide users into specific subsections.
Yes, I agree... since this is quite a major change to the appearance of the docs, there are certainly a few additional changes required to make everything look and work as expected. However, I believe that this is a good way forward and the docs will be much easier to navigate if this is done properly (... and the dark-theme switch is nice!)
I've modified the pytest code related to the docs section, and it's now running properly. There should be no issues now.
Great, thanks!
This is my first pull request, so the number of iterations might be a bit high. If there are any other problems, please let me know.
No worries on the iterations! Good thing takes time and we're all working in our free-time here. If we go forward with switching to the pydata-theme I definitely want to do it properly!
I am happy to assist with this PR as much as I can until it's ready to be merged!
I did a first quick review and so far most of the chapters look OK and the overall appearance and usabilty is very nice! Thanks again for pushing this forward!
Please remove the docs/source/api/autodoc_additional_props.rst
file as well (it is also automatically created during the build-process and should not be part of the repo). After that the tests should finally succeed.
And here's a list of suggstions I think would be nice to have... let me know what you think!
[ ] The logo on the titlepage somehow has a white background in the dark-theme... can you make that transparent?
[ ] There seems to be a mixup with the sectioin header levels in the sidebar
[ ] Is it possible to remove the gray background color for the shapes-grid on the titlepage ?
[ ] The User Guide already works really nice. However, the sections "Installation", "Examples", "API Reference" and "FAQ" somehow act like "standalone files" with an empty "Section Navigation"... Is it possible that they are part of the section hirachy?
... and one general question: I see that you allow changes by maintainers for this PR. Is it OK for you if i push changes directly to this PR (e.g. the associated branch of your fork) or would you prefer if i suggest edits as separate PRs to your fork?
I'm comfortable with both. This is my first pull request, and I'm still getting familiar with the workflow of pull requests. Currently, I'm in the process of refactoring the example section, taking inspiration from matplotlib. Once I'm done with the modifications, I'll push all the changes together.
Ok, perfect! Dont hesitate to get in touch if you have any questions or need help!
Hi @raphaelquast
In this version, I modified the Examples section to follow the Matplotlib style. As a result, I added the nbsphinx
library. However, this library conflicts with myst-nb
, so the previous Jupyter notebook can no longer be rendered proper. I changed two of them to .rst
format, but for the 'Jupyter Widgets' section, I couldn't find a suitable solution, so I haven't modified it yet. I looked at Matplotlib's Widgets section https://matplotlib.org/stable/gallery/index.html#widgets and found that their display is still in image form, without interactive features. May change it to a gif display like the other sections in the future?
Here are the answers to your questions:
The logo is now constantly displayed in the top left corner of the documentation, so I removed the logo from the homepage. Most PyData libraries' documentation follows this manner.
"There seems to be a mixup with the section header levels in the sidebar." This displays the page TOC, and was caused by mixed usage of section levels in the rst files. I have corrected this.
"Is it possible to remove the gray background color for the shapes-grid on the titlepage?" This was due to an extra space before shapes-grid. I've removed it, and now it displays correctly.
The "empty Section Navigation" is normal when there's only one file; only the right side shows the page TOC. Matplotlib's documentation is the same way (https://matplotlib.org/stable/users/release_notes.html). Section TOC only have constants when more files are added.
Hi @raphaelquast
The pull request failed to upload coverage to Codecov. Do you have any suggestions on this issue? I only made changes to the docs, and both documentation tests have passed. Theoretically, these changes shouldn't result in any test failures.
Hey, again thanks for your work!! I am a bit short on time right now, i'll try to review your new changes in the next few days!
2 quick comments:
Codecov servers can be a bit picky sometimes... A re-run usually does the job...
From your comment i see that you implemented a switch from myst_nb
to nbsphinx
...
The use of myst_nb
was quite intentional as it provides (in my opinion) nicer outputs and it allows for more extensive controls (hidden input/output cells, caching, myst markdown syntax in md-cells etc.)
Is there any particular feature that you require from nbsphinx
that is not provided bymyst_nb
?
If not, i'd very much like to keep using myst_nb
for rendering notebooks...
Hi @raphaelquast
I have discovered two libraries that can be used for creating thumbnail galleries: Sphinx-Gallery and nbsphinx.
Sphinx-Gallery
library relies on provided Python scripts (should work well with myst-nb
)nbsphinx
library relies on provided Jupyter notebooks.make html
command. I have created a new branch at https://github.com/Fanchengyan/EOmaps/tree/pydata_docs_gallery, where you can investigate the cause of the error.nbsphinx
successfully generates the gallery page. During my local testing, I discovered that the test_set_extent_to_location
function throws an error due to the unavailability of nominatim.openstreetmap.org. I have set it to mask.skip
. May this push could pass the CI.Hey, sorry for the delay!
I had a look at the pydata_docs_galery
branch and the error is ineed quite strange and i'm not sure its related to myst_nb
at all...
I was able to circuumvent it by changing the way how Maps.BM
is instantiated, but I still got a lot of warnings and finally ran into another error: Handler <function update_and_remove_templates at 0x000001EAD82AA320> for event 'html-page-context' threw an exception (exception: navbar-logo.html)
which I'm not yet sure where it is coming from...
At this stage I took a step back and started to think that we have to avoid that this PR is getting out of hand... We shouldn't try to do all at once here in a single PR (this way it'll never finish...)
Let's keep it simple and do one thing at a time? My suggestion would be the following:
pydata theme
and I welcome a bit of restructuring of the docs to make it more accessible.myst_nb
and I'd also avoid using sphinx_galery
for now.So for this PR, I'd suggest to only focus on switching the theme (and maybe a bit of the structure) without any major changes in the content of the docs.
Does this sound OK for you?
... and just to be clear: In general I really appreciate your work here and I do support any efforts in making the docs even better! We can implement a proper galery at a later stage as a separate PR once the new theme is properly implemented.
This sounds ok to me. I'll take a look later to see if there's a better option that is compatible with myst-nb
and can also display a gallery. Now, what do I need to do? Should I roll back to a certain commit or submit a new PR?
Hi @raphaelquast
This commit removes nbsphinx
; I wrote a simple script to automatically generate the gallery, so it now can continue using myst-nb
. Currently, this script only supports ipynb
files; it can be expanded to support md
files in the future.
Hey @Fanchengyan Really nice work! Looks like we can get the best of both worlds after all!
A quick questions before I try to find some time for an in-depth look at your changes:
Hi @raphaelquast
This is just an attempt to pass the CI. You can ignore it. The CI was failing consistently before. I saw the documentation mentioned updating the baseline, so I tried to update it, but there was no actual difference. The only difference between the two was in the font outlines.
Tests fails are unrelated... I have to fix the upload token for codecov then all should be fine! I give my best to have a thorough look at your changes as soon as possibe!
Hey, just a quick follow-up on the codecov-fails (haven't found the time to do a proper review yet) . The problems occur due to the fact that actions triggered from forks do not have access to repository-secrets (e.g. in this case the codecov upload token).... (more info here)
In general, codecov uploads from forks should automatically use a tokenless upload, but it seems this is no longer working as expected. I've now updated the config to use the latest codecov-upload action (v4).
Can you please rebase to the latest "dev" branch to see if this fixes the upload problems?
OK, I'll have a try.
Hi @raphaelquast
I've merged the latest development code and created a separate library for gallery generation: available at https://myst-sphinx-gallery.readthedocs.io/en/latest/index.html
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 75.15%. Comparing base (
478ca81
) to head (f732bf9
). Report is 68 commits behind head on dev.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
🚨 Try these New Features:
Hey @Fanchengyan
I looked at your canges and in general the docs look really nice.
However, I'm a bit worried on the galery still. Again, don't get me wrong here... its a marvellous idea in general, but I think there are some things that need to be considered:
Specifically, I have 2 concerns with the current approach:
Having a completely fresh package (that is not yet available on conda-forge) as a required dependency for EOmaps docs generation might create problems in the long run once updates are required etc. You might be motivated to create the package right now, but are you commited to maintaining it for years to come?
While it is of course the way to go in general to make the functionality available for others as well, I'm still a bit reluctant to have it as a mandatory dependency to build the docs right away.
My second concern is the fact that the galery is now effectively re-built with every attempt to create the docs. While this is nice to make sure all figures are up to date, it also requires a lot of computational resources which can be problematic, especially when relying on free services provided for open-source packages. (e.g. creation runtime on readthedocs is limited...).
Also adding several dependencies as requirements to build the docs complicates the build process. (version conflicts, longer creation time of the environment etc.)
And finally, the generation of figures that rely on the availability of webmap services might fail randomly due to server issues etc. (as you already encountered with the nominatim issue)
All of this was the reason why I initially opted for a "manually created static galery" approach that is only re-created locally if necessary.
Now, putting my concerns together, I think that a dynamic galery will create too many issues in the long run, but a doable approach could be that we use your myst-sphinx-galery
package to create a static galery locally and then add the created files etc. to the repository (and update if necessary)? What's your opinion on this approach?
We would have to include a short paragrph on how to re-create the galery in the "contribution-guide" but it would save a lot of resources and maintain a fast build time for the docs with as little dependencies as possible.
Hi @raphaelquast
I personally believe there is no issue with using MyST Sphinx Gallery
to generate a gallery:
sphinx-design
to display the grid and PIL
to generate small-sized thumbnails (to speed up webpage loading). I have basically implemented the functionality, and it should not require much maintenance in the future. However, I will continue to pay attention to this library, and if anyone proposes new requirements, I will consider incorporating them. Moreover, I have considered forward compatibility with each update, so this library should be quite stable.Sphinx Gallery
, this library does not execute any code. It merely reads the images referenced in the files for thumbnail generation, providing a top-image for the grid-item-card in the gallery, which is essentially static. In fact, the current gallery files have all been converted to rst files, and the images are ready-made; the code within them is not executed. Even for Jupyter notebooks, the image from the code cell output can be directly extracted, as long as it has been run locally once. Therefore, there is no need to worry about occasional failures in execution. You can refer to Thumbnail Strategies for more information.EOmaps
takes about 13 seconds. This is the time it takes when the EOmaps
gallery mostly consists of animated images (each frame requires generating a thumbnail, and most animated images in EOmaps have a frame number over 100), without animated images, the time is almost negligible.Hi @raphaelquast
In this version, I switched myst-sphinx-gallery
from pip
to conda
and pinned the version to 0.2.2. This way, even if there are issues with subsequent updates, it won't affect this library. Besides, I have added two new feature to myst-sphinx-gallery
in version 0.2.2:
Additionally, I made some adjustments to the documentation style of EOmaps
to make it more visually appealing:
pydata-sphinx-theme
is a bit small.Finally, I still recommend using myst-sphinx-gallery
because I've added many new critical features based on the previous code. For instance, previously, I just simply referenced the example images directly, which had two issues:
myst-sphinx-gallery
, however, can automatically generate thumbnails to a consistent small size (Default: (320, 224)), enhancing visual appeal and ensuring optimal page load speed. Besides, it can easily customize the appearance of the gallery and thumbnails using a variety of configuration options.
Hey @Fanchengyan
sorry for my late response!
I'm really grateful and amazed by your work here! Nice to see that you already managed to put myst_sphinx_galery
on conda-forge!
After consideration of your comments I agree that using myst_sphinx_galery
provides a lot of benefits compared to the current approach and I'm happy to use it in EOmaps!
I checked your changes and added some minor comments in the code. In addition, I noticed the following issue:
And finally, since none of your code should affect the baseline images for the unittests, can you please remove the updates to the tests/baseline
folder from the PR?
Thanks again for all your work here! The new docs look amazing!
Oh and a design question/suggestion... I think the emojis in the header are not particularly nice... I'd suggest to remove them. What's your opinion on this?
Perfect! The landing-page looks much cleaner now! From a first quick look this seems to be ready to be merged! 🥳
Any pending changes from your side still?
This PR looks fine to me now. However, I have a broader suggestion:
Consider duplicating some key examples from the User Guide in the Example Gallery in the future. This would make it easier for users to find relevant examples quickly. In my experience with matplotlib, I often directly search in the examples rather than in the User Guide. User Guide can be linked to each example for users who need more detailed information.
This PR looks fine to me now.
OK nice! Then I'll try my best to merge this as soon as possible and then release a new version including some other pending changes/fixes!
Consider duplicating some key examples from the User Guide in the Example Gallery in the future. This would make it easier for users to find relevant examples quickly. In my experience with matplotlib, I often directly search in the examples rather than in the User Guide. User Guide can be linked to each example for users who need more detailed information.
Yes, I fully agree on that part... I hope to find some time in the future to also extend the galery to include both more simple examples that just show a specific feature as well as extensive examples showing how far you can go with EOmaps. The new galery is definitely a big step forward to make this a reality since the thumbnails now allow having a lot of images in the galery without slowing down the browser too much!
Hey, I made some minor adjustments to fix remaining issues:
Updated dependencies to latest versions
Fixed remaining sphinx-build warnings
Fixed decrease in code-coverage due to no longer tested examples and doc-codeblocks
All code-blocks found in the documentation that have a tag ":id: test_...." are automatically parsed and run as unittests. The re-structuring of the docs broke this since previously it was assumed all source files are in the same folder. The same is true for the examples (
.py
files have previously been used to auto-generate unittests for each example) Now a recursive glob-pattern is used to ensure all files are properly checked for codeblocks.
I did some more re-structuring and fine-tuning of the docs:
User guide
in favor of a richer landing-pageI think with those final changes the new docs are ready to be released!
Thanks again for all your work here! It really helped to make the docs a lot better!
Its done! 🥳 Big thanks for all your efforts here! Hope we can collaborate again in the future!
The main modifications to the documentation include:
sphinx_rtd_theme
topydata_sphinx_theme
, resulting in a more visually appealing and elegant documentation.matplotlib
documentation.