-
Notifications
You must be signed in to change notification settings - Fork 308
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
Replace open3d with pyvista #663
Replace open3d with pyvista #663
Conversation
Need to figure out how to get the plotting to run headless within the CI container. That seems to be what's causing tests to fail. |
Did you ever open an issue with the developers? Can you link to it here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stoked to see you all switching to PyVista! Here is some feedback and hopefully my pointers will get the tests working/passing for each platform
A couple things:
Thanks for the assist @banesullivan. I'm really enjoying the simplicity of working with |
def plot_las(self, index: int, colormap: Optional[str] = None) -> Any: | ||
def plot_las( | ||
self, index: int | ||
) -> "pyvista.Plotter": # type: ignore[name-defined] # noqa: F821 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the deal with these ignores?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I actually need to update the output type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ignores are because we define pyvista inside the plot_las
method so mypy and flake8 are complaining that pyvista is not defined
@@ -595,27 +594,12 @@ def plot_las(self, index: int, colormap: Optional[str] = None) -> Any: | |||
points: "np.typing.NDArray[np.int_]" = np.stack( | |||
[las.x, las.y, las.z], axis=0 | |||
).transpose((1, 0)) | |||
point_cloud = pyvista.PolyData(points) # type: ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here, would be happy to add type hints for this if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if I import from pyvista import PolyData
and change the line to point_cloud: PolyData = PolyData(points)
mypy still complains Module "pyvista" has no attribute "PolyData" [attr-defined]
vis.close() | ||
vis = dataset.plot_las(index=1, colormap=None) | ||
vis.close() | ||
pyvista = pytest.importorskip("pyvista", minversion="0.35.1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isaaccorley do you remember why our testing minversion is different from our setup.cfg min version? I tried correcting this in #1276 but it's failing. Is 0.35.1 the real min version that works? I don't want to claim to support versions we can't test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be an oversight on my part. I don't remember the reason exactly. I recall there was something to do with certain wheels for different OS not being available maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wheels are pure Python, but could be limited by VTK. I'm going to try to find the oldest version for which our tests actually pass. We haven't been testing pyvista at all in our minimum version CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but could be limited by VTK
Most likely the case
I'm going to try to find the oldest version for which our tests actually pass.
I don't recommend this (see above comment #663 (comment)). Once you go back to older versions of PyVista (or let users do this), you'll need to limit VTK carefully. We work tirelessly to make sure the latest PyVista is compatible with all of VTK 9+, but older versions of PyVista may outright fail on import with newer versions of VTK. I recommend choosing a fairly recent version of PyVista as the minimum for this reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Managed to get PyVista 0.29 working fine without having to mess with VTK: #1276
* remove open3d add pyvista * refactor plotting pcl with pyvista * refactor tests with pyvista * remove open3d * remove skipping pcl plot test on mac * fix * remove unused import * add pyvista docs * use xvfb for pyvista tests * changes per suggestions * changes per suggestions x2 * remove bugged test * Test minimum pyvista version * mypy fix * More dep file updates * Document change Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Depends on pyvista/pyvista#3692
Refactors point cloud plotting in the
torchgeo.datasets.IDTReeS
dataset to usepyvista
instead ofopen3d
Note: Currently figuring out the min version which passes tests.
See #662
Example:
Screen.Recording.2022-07-10.at.12.28.10.PM.mov
Closes #932