Skip to content

BUG: Fix keyword param point_set -> point_sets#607

Merged
thewtex merged 8 commits intoInsightSoftwareConsortium:mainfrom
bnmajor:thinplatespline-fixes
Jan 28, 2023
Merged

BUG: Fix keyword param point_set -> point_sets#607
thewtex merged 8 commits intoInsightSoftwareConsortium:mainfrom
bnmajor:thinplatespline-fixes

Conversation

@bnmajor
Copy link
Collaborator

@bnmajor bnmajor commented Jan 24, 2023

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@bnmajor bnmajor requested a review from thewtex January 24, 2023 17:56
Copy link
Member

@thewtex thewtex 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 this should be point_set -- so adjustments may need to be made to the view code?

In 0.X, we could pass a list of point sets (and geometries), however in 1.X, I would like to support a single point set and geometry for view because it avoids ambiguities with NumPy-derived data types and lists and it provide a more reasonable way to specify parameters -- they can just be passed as kwargs to view. To add more data the scene, viewer = view(point_set1); viewer.add_point_set(point_set2) Does that make sense?

@bnmajor
Copy link
Collaborator Author

bnmajor commented Jan 24, 2023

@thewtex Makes sense! I'll just update this PR to make those fixes instead

@bnmajor bnmajor force-pushed the thinplatespline-fixes branch from e918fab to af07afc Compare January 24, 2023 22:21
@bnmajor
Copy link
Collaborator Author

bnmajor commented Jan 24, 2023

@thewtex Any preference on the default behavior for detecting the input type for numpy arrays, zarr groups, dask arrays, PyTorch tensors, or xarray DataArrays or Datasets? In all of these cases the default is to treat the data as an image if not explicitly passed in with the point_set kwarg. So for your example viewer = view(point_set1) this would (for most data types) be treated as an image without point_set keyword.

Also right now the behavior in the itk-vtk-viewer api is to overwrite the existing point set rather than update the list. I could either update setPointSets to append the new data, or maybe it would make more sense to add a new function like updatePointSets or appendPointSet?

@thewtex
Copy link
Member

thewtex commented Jan 25, 2023

@bnmajor thanks!

@thewtex Any preference on the default behavior for detecting the input type for numpy arrays, zarr groups, dask arrays, PyTorch tensors, or xarray DataArrays or Datasets? In all of these cases the default is to treat the data as an image if not explicitly passed in with the point_set kwarg. So for your example viewer = view(point_set1) this would (for most data types) be treated as an image without point_set keyword.

In detecting the type without an explicit kwarg, can we have Nx3 (x,y,z) or Nx2 (x,y) imply point set, and image otherwise?

Also right now the behavior in the itk-vtk-viewer api is to overwrite the existing point set rather than update the list. I could either update setPointSets to append the new data, or maybe it would make more sense to add a new function like updatePointSets or appendPointSet?

Thinking about transitions, addPointSet would be ideal.


Image = Union[np.ndarray, itkwasm.Image, zarr.Group]
Point_Sets = Union[np.ndarray, itkwasm.PointSet, zarr.Group]
Point_Set = Union[np.ndarray, itkwasm.PointSet, zarr.Group]
Copy link
Member

Choose a reason for hiding this comment

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

unrelated to your changes here, but can we please fix regardless: Point_Set -> PointSet 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

Assume data with two dimensions and shape (N, 2) or (N, 3) is a point set, all
other data is assumed to be an image.
Provides an example of inferring that the data is a point set without having to
explicitly use the keyword argument.
@bnmajor bnmajor force-pushed the thinplatespline-fixes branch from af07afc to 4c2ac86 Compare January 26, 2023 22:55
@bnmajor
Copy link
Collaborator Author

bnmajor commented Jan 26, 2023

In detecting the type without an explicit kwarg, can we have Nx3 (x,y,z) or Nx2 (x,y) imply point set, and image otherwise?

Sure thing! Updated with those changes now 👍

Thinking about transitions, addPointSet would be ideal.

Awesome. This branch now relies on Kitware/itk-vtk-viewer#643. Once that is in the itk-vtk-viewer version will need to be bumped before this branch is ready.

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

🥇 💯

@thewtex thewtex merged commit 222c172 into InsightSoftwareConsortium:main Jan 28, 2023
@bnmajor bnmajor deleted the thinplatespline-fixes branch February 14, 2023 21:32
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.

2 participants