fatiando / harmonica

Forward modeling, inversion, and processing gravity and magnetic data
https://www.fatiando.org/harmonica
BSD 3-Clause "New" or "Revised" License
211 stars 69 forks source link

Add conversion of prisms or a prism layer to PyVista objects #291

Closed santisoler closed 2 years ago

santisoler commented 2 years ago

Add a new visualization module that hosts prism_to_pyvista: a function to convert a set of prisms into a pyvista.UnstructuredGrid. Include the new module and this function in the API Reference. Add a new to_pyvista() method to the PrismLayer accessor that converts a prism layer into a pyvista grid, making it easier to plot it in 3D. The UnstructuredGrid has the information about each prism as hexahedrons, along with their physical properties as cell data. Add tests for the new features. Add pyvista and vtk as optional dependencies to environment.yml and setup.cfg. Add a new example for plotting a PrismLayer. Configure Sphinx to show pyvista plots in the gallery and to use the pyvista-plot directive in docstrings.

Reminders:

leouieda commented 2 years ago

@santisoler how about posting this to the PyVista forum to get feedback from the devs? https://github.com/pyvista/pyvista/discussions

santisoler commented 2 years ago

Sure! I would love to have feedback from them.

santisoler commented 2 years ago

Thanks a lot @banesullivan for the review! I really appreciate it!

I'm stepping ahead of time, but after this PR is merged I want to start drafting a similar way to plot tesseroids with pyvista. That might be more challenging since their faces are actually spherical or conical. I'll surely ping you when I have a draft :eyes:

leouieda commented 2 years ago

@santisoler I'm wondering if a new subpackage is really needed here. We're not planning on adding a bunch of visualizations to this package and this is a bit of an exception since we couldn't avoid it. Would it be better to leave that code in the prism_layer.py module for now? Or at least this subpackage should be private (_visualization) since we don't want people importing it directly.

santisoler commented 2 years ago

I'm retaking this PR after a long time!

@santisoler I'm wondering if a new subpackage is really needed here. We're not planning on adding a bunch of visualizations to this package and this is a bit of an exception since we couldn't avoid it. Would it be better to leave that code in the prism_layer.py module for now? Or at least this subpackage should be private (_visualization) since we don't want people importing it directly.

The way the prisms_to_pyvista function is written, it would allow users to pass any set of prisms and create an UnstructuredGrid that represent them. To do so, we should allow them to use this function as hm.prisms_to_pyvista or (not a fan) hm.visualization.prisms_to_pyvista.

Also, as mentioned before, I'm planning to add visualization functions for tesseroids as well (converstion to PyVista objects), so the visualization module sounds a good place for them as well.

If we decide to only allow these conversions through the methods of PrismLayer (and the future TesseroidLayer), so I'm ok with making _visualization private. But since it's already defined for the general purpose of any random set of prisms, I think we should keep it public. What do you think?

santisoler commented 2 years ago

I've made the visualization module public after what we decided on the Fatiando Call of 2022-04-22.

santisoler commented 2 years ago

Merged even codecov was failing. We are missing the test in cases where pyvista is missing and cannot be imported. We can safely ignore that.

leouieda commented 2 years ago

🎉 can't wait to see this in the docs!