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

Add PyVista for 3D plotting in Python #29

Merged
merged 9 commits into from
Sep 27, 2019
Merged

Add PyVista for 3D plotting in Python #29

merged 9 commits into from
Sep 27, 2019

Conversation

banesullivan
Copy link
Collaborator

@banesullivan banesullivan commented Sep 19, 2019

Todo


This PR uses PyVista for creating VTK datasets in Python for immediate plotting. The pyevtk export code is still present to allow users to save VTK files in any Python environment. PyVista is a heavy dependency so these additions are implemented as optional features.

When complete, this will need to use "Squash and Merge"


PyVista allows you to create VTK data objects in memory really easily instead of saving them out to static files. Why might you want VTK/PyVista meshes in Python? So you can skip saving VTK files and visualize the datasets right in Python! Check out this example:

import pyvista as pv
from gstools import SRF, Gaussian
import matplotlib.pyplot as pt

# structured field with a size 100x100x100 and a grid-size of 1x1x1
x = y = z = range(100)
model = Gaussian(dim=3, var=0.6, len_scale=20)
srf = SRF(model)
srf((x, y, z), mesh_type='structured')

# Create the VTK mesh
mesh = srf.to_pyvista()

# Quickly plot in 3D:
mesh.plot()

download

# Use PyVista for some interactive plotting
p = pv.Plotter(notebook=False)
p.add_mesh_threshold(mesh)
p.show_grid()
p.show()

2019-09-19 13 10 31

Notes

I added a new method: to_pyvista().

@banesullivan
Copy link
Collaborator Author

AppVeyor build seems hung on the twine upload?

@LSchueler
Copy link
Member

Wow, thank you so much for the effort you already put into this project!

I haven't looked into the details yet, but your plotting routines look very intriguing indeed.
Depending on how heavy the dependencies actually become, we might just put the imports into the according functions, in order to only import them when needed.

@MuellerSeb
Copy link
Member

@banesullivan : Thanks for this astonishing PR!
The pictures speak for themselves.
I'd love to have such an interface to pyvista. But I would suggest another way:

  • as you said, pyevtk is lightweight and therefore I would keep it as the standard way of exporting
    (at least for the 1.1.x versions of GSTools)
  • make pyvista an optional dependency, with the following line in the setup.py:
    extras_require={"show": ["matplotlib", "pyvista"]}
    (this also adds matplotlib for plotting)
  • as @LSchueler proposed, import pyvista within the called method (maybe check for ImportError)
  • keep the CI the way it is, as for the 1.1.x versions we want to still provide support for 2.7 and >=3.4
    (we have in mind to drop py2 support from 2020 on as you commented in Stop supporting Python<3.6 #30)
  • in field/base.py:
    • I would just add the to_pyvista method, that returns the pyvista object (then users can continue with the pyvista infrastructure)
    • keep the export_vtk routine
  • in tools/export.py:
    • just add a to_pyvista function (taking a mesh_type argument)

I hope you are OK with this. We don't want to drop so much supported version for a visualization routine. At least at the moment.

What do you think?

Thanks again for your work and the nice pyvista package!

Cheers, Sebastian

@banesullivan
Copy link
Collaborator Author

Hi @MuellerSeb, thanks for all the feedback! I'll get back to this PR soon and clean it up so PyVista is simply added on top as an extra and rebase all these commits to leave the CI integration untouched.

Overall, I think what you propose is perfect

@banesullivan
Copy link
Collaborator Author

banesullivan commented Sep 24, 2019

@LSchueler and @MuellerSeb, I just rebased all of the commits and made the desired changes! I also updated the description in the first post (^^^), take a look and let me know what you think!

Notes:

  • pyevtk is still used as the default method for exporting VTK datasets so all Python versions are supported.
  • PyVista is now listed as an extra dependency with matplotlib in the setup.py for the plotting option.
  • If a user calls to_pyvista and PyVista is unavailable, they are prompted with a friendly ImportError
  • No changes to the CI integration or wheel building

@banesullivan
Copy link
Collaborator Author

Also, I added tests for both pyevtk and PyVista. The PyVista tests will not run unless PyVista is installed... so the CIs will not run those tests, but I could set that up for Python version greater than 3.4 if desired.

@banesullivan banesullivan changed the title Replace pyevtk with PyVista Add PyVista for 3D plotting in Python Sep 24, 2019
Copy link
Member

@MuellerSeb MuellerSeb left a comment

Choose a reason for hiding this comment

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

Thanks for your effort! I like the implementation. Only the new requirements.txt is making trouble on travis.
Cheers,
Sebastian

requirements.txt Outdated Show resolved Hide resolved
@MuellerSeb
Copy link
Member

Also don't forget to add yourself to the AUTHORS.md under Contributors! ;-)

@MuellerSeb
Copy link
Member

AppVeyor build seems hung on the twine upload?

You don't have access to our PyPI password, that is why both, travis and appveyor, are hanging on twine and the CI fails. So this is no problem.

requirements.txt Outdated Show resolved Hide resolved
Copy link
Member

@MuellerSeb MuellerSeb left a comment

Choose a reason for hiding this comment

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

I think we are done now! Thanks for the enchanting feature!

@banesullivan
Copy link
Collaborator Author

banesullivan commented Sep 25, 2019

Looking good to me! I look forward to using this package - I've been searching for a good kriging toolbox and I think I've finally found it! Having the ties to PyVista hopefully might bring in a few other users if not just myself!

@banesullivan banesullivan mentioned this pull request Sep 25, 2019
3 tasks
Copy link
Member

@LSchueler LSchueler left a comment

Choose a reason for hiding this comment

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

Another thanks for your work from my side!
I just added some very minor things I'd like to see changed before merging.

README.md Outdated Show resolved Hide resolved
gstools/field/base.py Show resolved Hide resolved
gstools/tools/export.py Show resolved Hide resolved
gstools/tools/export.py Show resolved Hide resolved
gstools/tools/export.py Show resolved Hide resolved
gstools/tools/export.py Show resolved Hide resolved
gstools/tools/export.py Show resolved Hide resolved
Copy link
Member

@LSchueler LSchueler left a comment

Choose a reason for hiding this comment

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

Thanks again for the changes!

@LSchueler LSchueler merged commit 4a2dd3e into GeoStat-Framework:master Sep 27, 2019
@MuellerSeb MuellerSeb mentioned this pull request Mar 20, 2020
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.

3 participants