Skip to content

Save image only when necessary#730

Merged
akaszynski merged 5 commits intomasterfrom
fix/do_not_always_screenshot
May 24, 2020
Merged

Save image only when necessary#730
akaszynski merged 5 commits intomasterfrom
fix/do_not_always_screenshot

Conversation

@akaszynski
Copy link
Copy Markdown
Member

@akaszynski akaszynski commented May 22, 2020

Plot Optimization Patch

While testing out VTK9, I discovered that the unit tests were much slower. It turns out part of the issue is that we're saving a screenshot when the show method is triggered in the Plotter class:

self.last_image = self.screenshot(screenshot, return_img=True)
self.last_image_depth = self.get_image_depth()

In VTK < 9.0, this adds a small overhead, but in VTK 9, the overhead is quite noticeable. In an effort to streamline pyvista regardless of the VTK version, this PR adds a the global variable BUILDING_GALLERY that is enabled while building the documentation. Here's the timings for vtk 8.1.2.

No screenshots

>>> pyvista.BUILDING_GALLERY = False
>>> timeit grid.plot(interactive=False)
134 ms ± 1.42 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

Screenshots

>>> pyvista.BUILDING_GALLERY = True
>>> timeit grid.plot(interactive=False)
201 ms ± 1.15 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

@akaszynski akaszynski requested a review from banesullivan May 22, 2020 22:11
@akaszynski
Copy link
Copy Markdown
Member Author

akaszynski commented May 22, 2020

@banesullivan, I'd like you to take a look at this since you originally implemented the sphinx_gallery. I've built the gallery and can attest that it still builds the images.

@akaszynski akaszynski changed the title Save image when necessary Save image only when necessary May 23, 2020
@akaszynski akaszynski mentioned this pull request May 23, 2020
Merged
7 tasks
@akaszynski
Copy link
Copy Markdown
Member Author

akaszynski commented May 23, 2020

@banesullivan,

I see _prep_for_close was added for fixing QtInteractor. Is there any reason why we can't move it there and not have it within BasePlotter? Seems unnecessary here.

@codecov
Copy link
Copy Markdown

codecov bot commented May 23, 2020 •
edited
Loading

Codecov Report

Merging #730 into master will increase coverage by 0.91%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #730      +/-   ##
==========================================
+ Coverage   84.44%   85.36%   +0.91%     
==========================================
  Files          34       34              
  Lines        9459     9468       +9     
==========================================
+ Hits         7988     8082      +94     
+ Misses       1471     1386      -85

@akaszynski
Copy link
Copy Markdown
Member Author

akaszynski commented May 23, 2020

Also, please note that unit testing is down by 50% on Mac OS.

@banesullivan, please take a look at this. I'd really like to get tests cleaned up.

@banesullivan
Copy link
Copy Markdown
Member

banesullivan commented May 24, 2020

_prep_for_close was added for fixing the qt stuff but I think it is needed by the BasePlotter to make sure we never try to capture a screenshot after a user has destroyed the GL context. I think I implemented it as a failsafe for all the different behavior when exiting the plotter on different OS

@banesullivan
Copy link
Copy Markdown
Member

banesullivan commented May 24, 2020

i.e.I think taking it out might cause Plotter to segfault when a user hits the exit button on Windows

banesullivan
banesullivan approved these changes May 24, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

I agree with the changes and noted my thoughts in #732 (comment)

@akaszynski
Copy link
Copy Markdown
Member Author

akaszynski commented May 24, 2020

i.e.I think taking it out might cause Plotter to segfault when a user hits the exit button on Windows

We should look into this later, but for the time being I'll leave it in since it's not going to affect our unit testing.

@akaszynski akaszynski merged commit 1b83053 into master May 24, 2020
@akaszynski akaszynski deleted the fix/do_not_always_screenshot branch June 4, 2020 17:35
@banesullivan
Copy link
Copy Markdown
Member

banesullivan commented Jun 5, 2020

From the surface, it looks like we are causing builds to fail downstream in discretize from this PR and the concern I had in #732 (comment).

pinging @jcapriot, @akaszynski. I will look into this more tonight to make sure this is the issue and try to patch a fix

See https://travis-ci.org/github/simpeg/discretize/jobs/694897879#L3023

@akaszynski
Copy link
Copy Markdown
Member Author

akaszynski commented Jun 5, 2020

They'll have to add

# necessary when building the sphinx gallery
pyvista.BUILDING_GALLERY = True

Within conf.py for documentation building.

@jcapriot
Copy link
Copy Markdown
Contributor

jcapriot commented Jun 5, 2020

Thanks for the quick response @akaszynski I'll add that in

@banesullivan
Copy link
Copy Markdown
Member

banesullivan commented Jun 5, 2020

A lot of projects are using PyVista with Sphinx gallery... requiring them all to update with that variable isn’t ideal.

I’ll play with this tonight if we can preserve the original behavior

@akaszynski
Copy link
Copy Markdown
Member Author

akaszynski commented Jun 5, 2020

We can preserve the original behavior with:

BUILDING_GALLERY = 'sphinx' in sys.modules

That way whenever sphinx has been imported (which always happens when building galleries), screenshots are always saved.

I'll make the hotfix right away.

@akaszynski akaszynski mentioned this pull request Jun 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@banesullivan banesullivan banesullivan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@akaszynski @banesullivan @jcapriot