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

Implement ipyvtk #983

Merged
merged 22 commits into from
Nov 7, 2020
Merged

Implement ipyvtk #983

merged 22 commits into from
Nov 7, 2020

Conversation

akaszynski
Copy link
Member

@akaszynski akaszynski commented Nov 4, 2020

Overview

This PR implements #824 without changing much. I tried to clean up #824, but ended up needing a fresh start since our tests were consistently failing. Instead, this PR just adds in an interactor and adds an option for ipyvtk-simple rather than replacing panel (which is still useful). At this point, there's still no clear winner, but with some performance improvements in ipyvtk-simple Pull 13, I think it's up there. Plus, now users can write one set of code for my applications that use both jupyterlab and a standard python console.

We can move some of the other doc changes over once people have played around with this, but I was hoping on keeping this a "beta" feature for a bit until we work out the bugs/implementation rather than going all out and replacing anything.

@banesullivan, feel free to add back in some/all of your work in #824. I wanted to keep this change light and then consider greenlighting it in a minor release or two, so I figured keeping it fairly undocumented was reasonable.

Resolve #285, resolve #364

Additional Notes

There are some API changes, the largest being that we're always starting the interactive renderer (off or on screen). This has some big implications:

  • Off screen renders on VTK 9.0 on Linux do not close, meaning that building docs results in a segfault on 9.0.X. You can get around this on doc building by simply building the docs twice; the first will encounter the segfault, the second will complete. On CI, I'm just limiting the max version to 8.1.2 until the next VTK release which includes a fix for this.
  • Our documentation examples can finally include widgets, and it looks great, especially with the axes.
  • No more checking if the interactive render has been created. Simplifies code.
  • Unit testing on VTK 8.1.2 on Linux with the interactive render results in a plotter being rapidly flashed for each plotter unit test. It's annoying.

Overall, the changes result in a better plotter for the average/power user, so I think we should move forward with this. Flashing/doc building woes are something I can deal with until the next release of VTK.

@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #983 (051cdc1) into master (ba5ab42) will increase coverage by 2.04%.
The diff coverage is 89.56%.

@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
+ Coverage   87.10%   89.14%   +2.04%     
==========================================
  Files          36       37       +1     
  Lines        9165    11385    +2220     
==========================================
+ Hits         7983    10149    +2166     
- Misses       1182     1236      +54     

@banesullivan
Copy link
Member

I will jump on this soon here - it is now a priority for me again. might be a day or two...

@banesullivan
Copy link
Member

I'd like this to land on the next release (I know I've flipped-flopped on this a few times but I've converged on adding it)

@akaszynski
Copy link
Member Author

I'd like this to land on the next release (I know I've flipped-flopped on this a few times but I've converged on adding it)

If we add it as a supplemental feature, there's no reason we can't add it.

@banesullivan
Copy link
Member

@pyvista/developers, I added a change here that adds the vtkRenderWindowInteractor to all Plotters (on or off screen). This is the only notable change here and I'd like buy-in from folks before merging that.

Why? This makes it easy to support widgets when plotting with ipyvtk_simple and resolves #285. Plus it makes the API more consistent and we no longer have to check hasattr(self, 'iren') all over the place

@akaszynski
Copy link
Member Author

82d4181 is causing the following failures locally on Ubuntu 18.04, Python 3.7, vtk==8.1.2:

FAILED tests/test_plotting.py::test_screenshot
FAILED tests/test_scraper.py::test_scraper
FAILED tests/test_plotting.py::test_save_screenshot[.png]
FAILED tests/test_plotting.py::test_save_screenshot[.jpg]
FAILED tests/test_plotting.py::test_save_screenshot[.tif]
FAILED tests/test_plotting.py::test_save_screenshot[.jpeg]
FAILED tests/test_plotting.py::test_save_screenshot[.tiff]
FAILED tests/test_plotting.py::test_save_screenshot[.bmp]

@akaszynski
Copy link
Member Author

@pyvista/developers, this one is ready for review. It's nice to see axes on our docs:

image

Copy link
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.

LGTM! Though I played a significant part here so maybe I shouldn't give it approval

This has me pumped!! 🚀

README.rst Outdated Show resolved Hide resolved
pyvista/__init__.py Show resolved Hide resolved
Co-authored-by: Bane Sullivan <banesullivan@gmail.com>
@banesullivan
Copy link
Member

One final thought. I think we should bring back the system flag on import that we had for panel and use it for ipyvtk_simple. This will make setting up the binder at playground.pyvista.org a bit easier.... will push a change momentarily

banesullivan added a commit to pyvista/cookiecutter-pyvista-binder that referenced this pull request Nov 7, 2020
@banesullivan banesullivan merged commit 46cbb6b into master Nov 7, 2020
@akaszynski akaszynski mentioned this pull request Nov 8, 2020
adeak added a commit to adeak/pyvista that referenced this pull request Nov 9, 2020
* upstream/master: (101 commits)
  Allow picking in notebook with ipyvtk (pyvista#996)
  Fix subplotting with ipyvtk_simple (pyvista#994)
  Use render window size (pyvista#995)
  version bump to 0.27.0 (pyvista#993)
  Add Docker Documentation (pyvista#992)
  Reorg Plotting Testing (pyvista#990)
  Allow Python 3.9 and Explain Building (pyvista#991)
  remove added xvfb line in init
  Revert "add ipyvtk-simple docs"
  add ipyvtk-simple docs
  add ipyvtk-simple docs
  remove viz of last picked cell after clicking outside the mesh (pyvista#984)
  Add widgets to examples gallery (pyvista#988)
  Implement ipyvtk  (pyvista#983)
  Add additional methods to PlotterITK (pyvista#980)
  get pop to behave like pop (pyvista#979)
  remove binding to our fly_to_mouse_position (pyvista#981)
  removed depreciated features (pyvista#978)
  Feat/unstr grid cell dict (pyvista#976)
  ensure scalarbar colors tracks the number of colors (pyvista#971)
  ...
@akaszynski akaszynski deleted the feat/alt_ipyvtk-simple branch November 10, 2020 19:53
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.

Interactive canvas within Jupyter Notebooks orientation axes for off screen plotting
2 participants