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 two minor issues causing noise in docs or doc build #2908

Merged
merged 3 commits into from
Jul 7, 2022

Conversation

adeak
Copy link
Member

@adeak adeak commented Jun 29, 2022

Found two minor issues while working on other, build-heavy PRs.

The first one (warning from read_legacy() doctest) I would've fixed in #2892 but we probably won't merge that. It makes more sense to fix these in a separate PR.

The second one is a subtle issue where our plot directive collects all existing plots, so if there's a dangling plotter it gets rendered too, even if it doesn't have any show() calls. (Not sure if we want to try to catch this; sounds too complex and too error-prone).

From https://dev.pyvista.org/api/plotting/_autosummary/pyvista.Plotter.set_background.html in devdocs:
screenshot: two example code bocks followed by a blank black then the intended normal figure

What I think happens is that the first plotter is kept open, so when we define a new plotter with name pl in the second test the first one is still around. We can either close the first plotter, or make sure to name the second plotter the same as the first one. I went with the former solution for now.

Needs:

@adeak adeak added the maintenance Low-impact maintenance activity label Jun 29, 2022
@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #2908 (6c50905) into main (47ddcd9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2908   +/-   ##
=======================================
  Coverage   94.30%   94.30%           
=======================================
  Files          76       76           
  Lines       16527    16527           
=======================================
  Hits        15586    15586           
  Misses        941      941           

@adeak adeak mentioned this pull request Jun 29, 2022
11 tasks
akaszynski
akaszynski previously approved these changes Jun 29, 2022
Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

This works. Thanks!

Looks like we angered the doc build.

@akaszynski akaszynski dismissed their stale review June 29, 2022 13:14

doesn't work

@adeak
Copy link
Member Author

adeak commented Jun 29, 2022

So close 😄 I haven't looked at the artifact yet.

@adeak
Copy link
Member Author

adeak commented Jun 29, 2022

Oh wow, this one plotter.close() line blows up everything. What the heck? Seems like something we should fix in the plot directive... I'll investigate 😅

executing simple.. [ 99%] user-guide/simple
executing vtk_to_pyvista] user-guide/vtk_to_pyvista
executing what-is-a-mesh] user-guide/what-is-a-mesh

/home/adeak/pyvista/plotting/renderers.py:docstring of pyvista.plotting.renderers.Renderers.set_background:37: ERROR: This plotter is closed and unable to save a screenshot.
/home/adeak/pyvista/plotting/renderer.py:docstring of pyvista.plotting.renderer.Renderer.set_environment_texture:32: ERROR: This plotter is closed and unable to save a screenshot.
/home/adeak/pyvista/plotting/renderer.py:docstring of pyvista.plotting.renderer.Renderer.set_focus:22: ERROR: This plotter is closed and unable to save a screenshot.
/home/adeak/pyvista/plotting/renderer.py:docstring of pyvista.plotting.renderer.Renderer.set_position:26: ERROR: This plotter is closed and unable to save a screenshot.
/home/adeak/pyvista/plotting/renderer.py:docstring of pyvista.plotting.renderer.Renderer.set_scale:36: ERROR: This plotter is closed and unable to save a screenshot.
[and so on...]

@adeak
Copy link
Member Author

adeak commented Jun 29, 2022

OK, one small piece of the issue is that plotter.close() doesn't remove the plotter from pyvista.plotting._ALL_PLOTTERS, only pv.close_all() does. But I guess we can't just remove closed plotters from _ALL_PLOTTERS because we need closed movie/gif plotters to stick around to use their gif files.

Side note: we also have a few gifs that are called .png. Probably sphinx gallery's doing:

>>> import pathlib
>>> import matplotlib.pyplot as plt
>>> dir = pathlib.Path('doc/_build/html/_images/')
>>> files = dir.glob('**/*.png')
>>> for file in files:
...     try:
...         img = plt.imread(file)
...     except:
...         print(file)
... 
doc/_build/html/_images/sphx_glr_orbit_003.png
doc/_build/html/_images/sphx_glr_reader_002.png
doc/_build/html/_images/sphx_glr_linked_001.png
doc/_build/html/_images/sphx_glr_gif_001.png
doc/_build/html/_images/sphx_glr_collisions_001.png
doc/_build/html/_images/sphx_glr_texture_007.png
doc/_build/html/_images/sphx_glr_orbit_002.png
doc/_build/html/_images/sphx_glr_orbit_001.png
doc/_build/html/_images/sphx_glr_image-fft-perlin-noise_006.png
doc/_build/html/_images/sphx_glr_isovalue_001.png
doc/_build/html/_images/sphx_glr_moving_cmap_001.png

I bet these are all the examples gallery gifs.

@@ -417,6 +417,7 @@ def set_background(self, color, top=None, all_renderers=True):
>>> plotter.set_background('black')
>>> plotter.background_color
Color(name='black', hex='#000000ff')
>>> plotter.close()
Copy link
Member Author

Choose a reason for hiding this comment

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

This will work after #2907.

Copy link
Member Author

@adeak adeak Jun 29, 2022

Choose a reason for hiding this comment

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

Output on top of #2907:

Screenshot 2022-06-29 at 19-55-09 set_background — PyVista 0 35 dev0 documentation

* upstream/main:
  Add setup and teardown functionality to plot_directive (pyvista#2907)
  Update hypothesis requirement from <6.48.4 to <6.49.2 (pyvista#2942)
  Bump lxml from 4.9.0 to 4.9.1 (pyvista#2936)
  Bump typing-extensions from 4.2.0 to 4.3.0 (pyvista#2939)
  Update hypothesis requirement from <6.48.3 to <6.48.4 (pyvista#2938)
  Bump trimesh from 3.12.6 to 3.12.7 (pyvista#2937)
  Update information of release (pyvista#2911)
  update pre-commit hooks (pyvista#2931)
  Handle invalid theme on init (pyvista#2917)
  Add `py.typed` Marker (pyvista#2904)
  [create-pull-request] update local intersphinx (pyvista#2926)
  Add tags with python commands to avoid misnumbering (pyvista#2416)
  Fix the url link of discretize (pyvista#2914)
  Add DataSet.cell_point_ids (pyvista#2897)
  Update hypothesis requirement from <6.48.2 to <6.48.3 (pyvista#2918)
  Ensure data remains a shallow copy when added to plotter (pyvista#2888)
@adeak
Copy link
Member Author

adeak commented Jul 6, 2022

CI and local build works, only error is due to #2901.

@akaszynski akaszynski added this to the 0.35.0 milestone Jul 7, 2022
Copy link
Member

@akaszynski akaszynski 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!

@akaszynski akaszynski merged commit 6bb2477 into pyvista:main Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants