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

Update OpenFOAM example to reuse Reader #2530

Merged
merged 1 commit into from
May 1, 2022

Conversation

MatthewFlamm
Copy link
Contributor

Overview

#2485 fixed OpenFOAMReader to be able to reuse the reader object for time steps. This PR updates the example to take advantage of this.

@github-actions github-actions bot added documentation Anything related to the documentation/website maintenance Low-impact maintenance activity labels Apr 29, 2022
@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #2530 (1f3883b) into main (b6ab604) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2530      +/-   ##
==========================================
- Coverage   93.72%   93.72%   -0.01%     
==========================================
  Files          75       75              
  Lines       16116    16120       +4     
==========================================
+ Hits        15105    15108       +3     
- Misses       1011     1012       +1     

Copy link
Member

@adeak adeak 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.

One barely related thing I noticed: in the built example some names get intersphinx links:

Screenshot from 2022-04-30 21-29-22

filename links to stdlib str which is fair enough. But pyvista.OpenFOAMReader links to abc.ABC which is not very helpful. Do you think we should try to investigate this?

@MatthewFlamm
Copy link
Contributor Author

filename links to stdlib str which is fair enough. But pyvista.OpenFOAMReader links to abc.ABC which is not very helpful. Do you think we should try to investigate this?

Other objects in the pyvista namespace are not linked in code blocks. This makes me suspect that the linking will look backwards into the inheritance until it finds a valid hit, in this case abc.ABC. object must not register in the same way?

@MatthewFlamm
Copy link
Contributor Author

Is there a way to get the linking to work for PyVista classes?

@adeak
Copy link
Member

adeak commented Apr 30, 2022

I have no idea what component is doing this, then again I don't really know sphinx or numpydoc. I tried an extension in #1845 but that didn't really go anywhere.

@adeak
Copy link
Member

adeak commented Apr 30, 2022

I wonder what happens if we add pyvista to the intersphinx mapping dict... 😄

@MatthewFlamm
Copy link
Contributor Author

🙃 . I can't find any of this linking in code blocks in the docstring examples. It seems to be contained in the examples section. I think that the examples are created before the package documentation, so this might be the root cause?

@adeak
Copy link
Member

adeak commented Apr 30, 2022

Good point, that might be it. Out of curiosity I'll see what happens if I try to add the intersphinx mapping from dev.pyvista.org. I expect our regular links to break (be redirected to dev.pyvista.org).

I think the feature we see is "global variable inspection" from sphinx-gallery. Haven't figured out what we can do about it (short of building the examples in a separate step if your hunch is correct).

@MatthewFlamm
Copy link
Contributor Author

There is also this section, but it is not explicitly configured here. https://sphinx-gallery.github.io/stable/configuration.html#add-intersphinx-links-to-your-examples

@adeak
Copy link
Member

adeak commented Apr 30, 2022

I could only find this related issue/feature request, concerning interconnected libraries. One workaround mentions building HTML twice but that* doesn't seem to help for me. I don't know if something like sphinx-doc/sphinx#3426 (comment) could work.

*I should clarify that I literally just tried make html a second time. I assume sphinx figured that everything is up to date. I expect that deleting the built docs (but not the built doc/_build/html/objects.inv file) would work.

Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

Good catch. LGTM.

@adeak
Copy link
Member

adeak commented Apr 30, 2022

For the record I deleted all of doc/_build/html, recreated doc/_build/html (just the directory) and put objects.inv from a full build back into it. Rerunning make html didn't fix the OpenFOAMReader intersphinx link I pointed out above.

@MatthewFlamm MatthewFlamm changed the title Update OpenFOAM example Update OpenFOAM example to reuse Reader May 1, 2022
@MatthewFlamm MatthewFlamm merged commit 7d14e69 into main May 1, 2022
@MatthewFlamm MatthewFlamm deleted the maint/update-openfoam-example branch May 1, 2022 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Anything related to the documentation/website maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants