fatiando / harmonica

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

Drop null prisms when converting a prism layer to pyvista #393

Closed santisoler closed 1 year ago

santisoler commented 1 year ago

Add an optional drop_null_prisms flag to the prism layer to_pyvista() method that ditches the prisms that have zero volume or nan values in their top or bottom boundaries. These null prisms won't be included in the PyVista Unstructured grid. By default the flag is set to True. Add a new test for this new feature.

Relevant issues/PRs:

Fixes #392

santisoler commented 1 year ago

This looks good and will definitely be helpful! One question. Why not drop null prisms by default? It shouldn't change the results, but it will avoid errors if you pass a prism layer with nan's.

Thanks! I wasn't 100% sure about it. I was thinking mainly when plotting a topography, it might be weird to have a "hole" where the prism has a zero thickness. Now that I put it into words, it's weird to say that I'm actually plotting a prism that has no volume 😕

We could drop the drop_null_prisms argument altogether and just check the prism layer for nans at each call to to_pyvista. Happy with your implementation just curious about your thought process.

Sure! I totally forgot that the top and bottom arrays could have nans. We should totally remove those prisms too. I'll push a commit to fix that right away.

santisoler commented 1 year ago

What do you think @mdtanker? Should we set drop_null_prisms=True as default?

mdtanker commented 1 year ago

Those changes look good. I'd vote for drop_null_prisms=True by default. I just tried with a few grids and they look sort of strange with the null prisms included.

santisoler commented 1 year ago

I've just changed the defaults to drop_null_prisms=True. @mdtanker, let me know what you think.

The documentation building is failing for some reason... I'll check that out before merging this.

mdtanker commented 1 year ago

Looks good, merge away!