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 points_to_cells and cells_to_points ImageDataFilters #6071

Merged
merged 29 commits into from May 19, 2024

Conversation

user27182
Copy link
Contributor

Overview

Add two new complimentary ImageDataFilters for converting how voxel data is represented.

Voxels for images are often represented as points with point data. But, confusingly, converting image data (with point voxels) to an unstructured grid or using pretty much any cell-based DataSet filter (e.g. threshold) will perform the operation on the voxel cells of the image data, not the points. This can result in unexpected output, e.g. a 2x2x2 array of voxel points, when cast to an unstructured grid, will be visually represented as a single voxel cell instead of the 8 input voxel points.

Using the filters from this PR enables the points to first be converted to voxel cells so that cell-based operation will generate the expected output.

This is a follow-up from #5968. Since the internal vtk filter for that PR requires voxel point data, the conversion filters from this PR will enable any inputs, with either point or cell data , to be used with that filter (i.e. cell data is first converted to point data using cell_voxels_to_point_voxels.

@pyvista-bot pyvista-bot added the enhancement Changes that enhance the library label May 11, 2024
Copy link

codecov bot commented May 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.99%. Comparing base (bcf7f89) to head (1d28963).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6071   +/-   ##
=======================================
  Coverage   96.99%   96.99%           
=======================================
  Files         141      141           
  Lines       24647    24691   +44     
=======================================
+ Hits        23906    23950   +44     
  Misses        741      741           

@user27182
Copy link
Contributor Author

@pyvista-bot preview

@pyvista-bot
Copy link
Contributor

Comment on lines 1015 to 1017
However, this does not visually show the correct representation when point data
is used to represent the centers of voxels. In this case we can convert the
point data to cell data to create a cell-based representation of the voxels.
Copy link
Member

Choose a reason for hiding this comment

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

I was reading the documentation and needed help understanding the process that is taking place here. Are you calculating the average of the data of the points in the cell and making it the data of the cell?

However, I would like to be able to understand the process that is being done just by reading the documentation. Therefore, I am writing my impressions without reading the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual point or cell data is not modified at all. It is passed through without modification.

The conversion is completely lossless in that regard. It is possible to convert the point voxels to cell voxels and back any number of times and the data will remain the same.

What this filter does is simply change how voxels are represented in terms of the actual ImageData container itself.

Basically, the filter simply modifies the container (I.e the ImageData itself, not the point or cell data), to change what the "definition" of a voxel is.

Many image libraries define an image as follows, for example:

image

In this case, the image is defined as an array of pixel cells, but the point data of the image is the center of the pixel (or voxel). So, in this case, a pixel or voxel is fundamentally "a point in the center of a pixel cell". Generally, when working with an array of point data, the points themselves represent the center points of the pixels or voxels.

This contrasts with how VTK fundamentally defines ImageData, where the points are the "corners" of cells, not the cell centers. So, if I have a single point voxel, what I really have is a point in the center of vtk Voxel Cell. So in order to correctly represent a single voxel point , I actually need a single vtk Voxel Cell. The confusing part is that this now means that I need 8 points (of the Voxel Cell) to represent the single point voxel that I actually have (which is at the center of the cell).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I have scalar data for a single voxel (an abstract voxel, not a vtk voxel cell), there are two distinct representations I can use. Both are valid representations but have different uses depending on the filter or application.

1: Represent this as a single vtk point, with a point data array of length one

2: Represent this as a single vtk voxel cell, with a cell data array of length one.

If you'd like I can provide more concrete examples using specific filters to demonstrate what I mean and show why the filters from this PR are useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For additional context about the notion of pixels as cells vs pixels as points, and the confusion surrounding it in VTK, see the vtk discourse discussion about it here:

https://discourse.vtk.org/t/proposal-to-add-orientation-to-vtkimagedata-feedback-wanted/120/44

Only the linked comment, and the replies after it are relevant (the rest of the discussion is about image orientation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This quote from Ken Martin summarizes what this PR does very nicely.

"People tend to expect 100 pixels at 1mm per pixel to create an image of 100mm, but in VTK 100 point samples on an axis equals 99 cells on an axis. And when you draw those cells they look like 99 pixels when people expect 100. I think moving to cell data for pixels would get us to a more common representation. The image would have 100 pixels of cell data, 100 cells, and 101 points defining the boundary of those cells (on an axis)."

point_voxels_to_cell_voxels will make it so that 100 point samples (with 99 cells) becomes 100 cells with 101 points.

cell_voxels_to_point_voxels does the reverse.

In either case, the data itself is the same, it's just a matter of representation. And this difference in representation will impact how the filter processes the data. If the wrong representation is used, this can give incorrect or unexpected results with some filters.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. That was a very clear explanation. I will merge once #6071 (comment) is resoleved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I will rename the filters and push the updates soon, within the next day or so

@user27182 user27182 changed the title Add point_voxels_to_cell_voxels and cell_voxels_to_point_voxels ImageDataFilters Add points_to_cells and cells_to_points ImageDataFilters May 14, 2024
@user27182
Copy link
Contributor Author

@pyvista-bot preview

@pyvista-bot
Copy link
Contributor

@MatthewFlamm
Copy link
Contributor

MatthewFlamm commented May 14, 2024

This looks really useful. I haven't looked at the code in detail, but it is my impression that the other kind of data would be discarded completely? For example, I have ImageData with point data array a and cell data array b. If I use points_to_cells, then b will be completely lost. This should be documented.

cells_to_points should be linked to and differentiated from cell_data_to_point_data. You are remeshing to preserve the data itself rather than preserving the mesh and performing some interpolation. Similar for the opposite direction.

@user27182
Copy link
Contributor Author

This looks really useful. I haven't looked at the code in detail, but it is my impression that the other kind of data would be discarded completely? For example, I have ImageData with point data array a and cell data array b. If I use points_to_cells, then b will be completely lost. This should be documented.

Good point, yes this is correct, the 'other' data is lost. It was previously documented, e.g. for points_to_cells the docstring stated "Any cell data at the input is ignored and is not used." This was unintentionally removed in a4b2410 which was a big overhaul of the docs. I'll make sure to include it again.

cells_to_points should be linked to and differentiated from cell_data_to_point_data. You are remeshing to preserve the data itself rather than preserving the mesh and performing some interpolation. Similar for the opposite direction.

Do you mean to add more information to the See Also sections just to make it clear what the differences are and/or make it clear why these other methods are linked?

You are remeshing ...

I like the term "remeshing". I wasn't sure if I should use the word "convert", "transform", "modify", etc... I think perhaps "remeshing" should be used in the docs instead. Let me know if you agree, any input to make this method as clear as possible is very helpful.

@MatthewFlamm
Copy link
Contributor

Do you mean to add more information to the See Also sections just to make it clear what the differences are and/or make it clear why these other methods are linked?

If it can be stated succinctly, it makes sense to me to have in See Also. I think these methods will get confused unless directly compared/contrasted.

I personally like remeshed as it gets to the essence of the operation. You preserve the point/cell data but place it on a new mesh. I would look for other opinions though...

@user27182
Copy link
Contributor Author

@pyvista-bot preview

@pyvista-bot
Copy link
Contributor

@bjlittle
Copy link
Contributor

I personally like remeshed as it gets to the essence of the operation. You preserve the point/cell data but place it on a new mesh. I would look for other opinions though...

I'd vote for remesh as a concept i.e., the data is preserved "as-is". I think the clarity would be super useful to the user.

In a cartographic context regrid involves a change in resolution of the mesh/grid and/or coordinate reference system (CRS) with appropriate interpolation of the data.

Using these two terms (do we have a glossary?) consistently throughout the codebase and docs would be helpful, if you all agree with what they mean.

MatthewFlamm
MatthewFlamm previously approved these changes May 16, 2024
Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

I don't have time for a full review of this, but the parts I mentioned before look good to me. Thanks!

@user27182
Copy link
Contributor Author

@pyvista-bot preview

@user27182
Copy link
Contributor Author

@pyvista-bot preview

@user27182
Copy link
Contributor Author

@pyvista-bot preview

@user27182
Copy link
Contributor Author

@pyvista-bot preview

@pyvista-bot
Copy link
Contributor

@user27182
Copy link
Contributor Author

user27182 commented May 17, 2024

The new standalone example looks good except there is a bug somewhere with image_threshold. See images below. I'll need some time to track this down and fix it.

In the interim I could create a separate PR for adding this example. That way the actual filters can still be merged.

I am getting this image locally
image

but this is showing up in the docs for some reason:

image

@user27182
Copy link
Contributor Author

@pyvista-bot preview

tkoyama010
tkoyama010 previously approved these changes May 19, 2024
Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

LGTM

@tkoyama010
Copy link
Member

@pyvista-bot preview

@pyvista-bot
Copy link
Contributor

@tkoyama010 tkoyama010 enabled auto-merge (squash) May 19, 2024 12:56
@tkoyama010 tkoyama010 merged commit 432a1eb into pyvista:main May 19, 2024
27 checks passed
@user27182 user27182 deleted the feat/convert_voxels branch May 19, 2024 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Changes that enhance the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants