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

Interactive pyvista scenes in examples #826

Closed

Conversation

ChristosT
Copy link

The changes in pyvista/pyvista#4938 may not be easily applicable to the examples section of the pyansys-geometry documentation:

pyvista uses sphinx-gallery along with a custom scraper for reading the source of an example from a .py file and generating a .rst files that embeds a custom offlineviewer directive.

We could convert all examples from jupyter notebooks to .py files and use pyvista infrastructure to generate the examples gallery .

As an alternative we can add a simple snippet after every plot.show() that extracts the scene as html and embeds it in the documentation as an iframe i.e.

scene = pl.export_html(filename=None)

from IPython.display import display, HTML
html_str = scene.getvalue().replace('"','"')
iframe=f"""
<iframe
    srcdoc="{html_str}"
    width='100%'
    height='500'
</iframe>
"""
HTML(iframe)

of course we do not want to show all this code to the user so, we can use cell tags to hide it . See the modified .mystnb file.

However, these tags come with a couple of requirements.

  • Switching to the myst_nb parser from jupytext that is currently being used. I am not sure why. jupytext should support these tags but in my experiments it does not. Maybe I am missing some configuration.

  • The version of myst_nb from pypi is incompatible with the dependencies of this project (limits sphinx version among others) so we need to install the latest from git. (A new release should come soon see here )

With the above caveats here is one example:

pyansys_geometry_documentation_00-2023-10-24_20.09.12.mp4

(remember to start a html server if you try this pull request locally python -m http.server --directory doc/_build/html)

@AlejandroFernandezLuces

@ChristosT ChristosT requested review from PipKat and a team as code owners October 25, 2023 00:14
@AlejandroFernandezLuces
Copy link
Contributor

Thanks for the work @ChristosT 😄

We really appreciate having the option to have interactive plotters. However we have some concerns with the maintainability of this approach:

  • Copy pasting the same code across all documentation seems a bad idea to me. I think this would greatly increase the maintenance workload in the long term. Isn't there any way to better integrate this with the plot function of pyvista, or with the documentation tools that we are using?
  • sphinx dependency is a really delicate dependency for us. A lot of other libraries, including some of our documentation libraries (e. g. ansys-sphinx-theme), depend on sphinx, so we have to be careful and pin this dependency on a version that works. I see that you unpinned it, so I assume you've had problems with versions. We really need to avoid downgrading or unpinning sphinx or else other libraries might have compatibility issues.
  • Also, regarding myst-nb, we prefer to avoid installing the dev version of any dependency. If we would pursue this approach, we would have to wait until that working version is released.

Overall I think this is in the right direction, we just need more maintainability in the solution for it to be stable in the long term. Let me know what do you think about these points and feel free to reach me if you have any doubt.

Thanks again for the hard work you are doing 🚀

@ChristosT
Copy link
Author

Thank you for the quick review !

regarding the first of your points I totally agree about the extra burden I will be looking into it. Ideally we could move the extra code to the return value of plot.show(return_iframe=True) or have a utility function to do it or inject it in your documentation automatically.

As for the other two. The sphinx update was by mistake we can use the pinned version you already have same goes with myst_parser as for myst_nb the timing is perfect as they have a rc release today https://pypi.org/project/myst-nb/1.0.0rc0 which works with this branch !

@AlejandroFernandezLuces
Copy link
Contributor

That's perfect, I'm glad that dependency versions are not a problem.

A suggestion, if you plan to integrate the code into the plot function, I think it might be a good idea to create a global configuration flag in PyVista instead of using a parameter, something like pv.PLOT_DOCUMENTATION. This way, we can activate that flag in the conf.py file and we wouldn't have to modify the examples at all.

@ChristosT
Copy link
Author

working with pyvista to integrate this into plot itself pyvista/pyvista#5168

@AlejandroFernandezLuces
Copy link
Contributor

working with pyvista to integrate this into plot itself pyvista/pyvista#5168

Great! Thanks for working on this 🙂

@PipKat
Copy link
Member

PipKat commented Nov 2, 2023

I believe you can get rid of all the "bad" Vale warnings (meaning they shouldn't even be occurring), by updating the Documentation Style Check in the ci_cd.yml file to note a Vale version:

docs-style:
name: Documentation Style Check
runs-on: ubuntu-latest
steps:
- name: PyAnsys documentation style checks
uses: ansys/actions/doc-style@v4
with:
token: ${{ secrets.GITHUB_TOKEN }}
vale-version: "2.29.5"

@Revathyvenugopal162 for confirmation!

Copy link
Member

@PipKat PipKat left a comment

Choose a reason for hiding this comment

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

Hi, @ChristosT I don't have the technical knowledge to review this PR. I'll leave that to Alex and Roberto!

@RobPasMue
Copy link
Member

I believe you can get rid of all the "bad" Vale warnings (meaning they shouldn't even be occurring), by updating the Documentation Style Check in the ci_cd.yml file to note a Vale version:

docs-style: name: Documentation Style Check runs-on: ubuntu-latest steps: - name: PyAnsys documentation style checks uses: ansys/actions/doc-style@v4 with: token: ${{ secrets.GITHUB_TOKEN }} vale-version: "2.29.5"

@Revathyvenugopal162 for confirmation!

Hi @PipKat - yes we are aware of these Vale issues. We are pending their resolution. Vale checks have been deactivated on the main branch. I am curious why they have kicked in here.

@RobPasMue
Copy link
Member

Oh now I see - the fork is outdated.

Looks related to the trimmed version of the documentation we are using
relevant warning:
pyansys-geometry/doc/source/api/ansys/geometry/core/designer/face/index.rst:471:duplicate object description of face.edges, other instance in api/ansys/geometry/core/designer/face/index,
The current makes the code-block and the rendered scene  too narrow
@ChristosT
Copy link
Author

@AlejandroFernandezLuces this now works with the latest master of pyvista. The new pyvista release is coming soon.

@AlejandroFernandezLuces
Copy link
Contributor

I'll give this a try and I'll let you know my feedback, thank you for implementing this 🙂

@AlejandroFernandezLuces
Copy link
Contributor

I added your changes to my local, unfortunately we are still facing issues. When rendering with HTML backend, we get the interactive scene in the docs, but it is always empty. In this screenshot, I ran the same code in a python terminal and in a Jupyter Notebook with HTML backend.

I'll triple check to see if I'm missing anything, but it seems to me that this is caused from PyVista side. Any ideas of what might be happening?

image

@ChristosT
Copy link
Author

Any ideas of what might be happening?

Does it work if you have a single object ?
This might be relevant pyvista/pyvista#5361

@RobPasMue
Copy link
Member

Any news here @AlejandroFernandezLuces ?

@AlejandroFernandezLuces
Copy link
Contributor

Does it work if you have a single object ? This might be relevant pyvista/pyvista#5361

The problem seems to happen with MultiBlock objects only. If I add a PolyData object it seems to work fine. However, if I add only one MultiBlock it doesn't show anything. Using the Python viewer everything works as expected, so I think this is on PyVista side. If you have any idea or need me to test anything let me know 🙂

@RobPasMue RobPasMue deleted the branch ansys:feat/simplified-docs April 2, 2024 06:47
@RobPasMue RobPasMue closed this Apr 2, 2024
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

4 participants