-
-
Notifications
You must be signed in to change notification settings - Fork 663
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
ENH: Support for Mesh serialization #3154
ENH: Support for Mesh serialization #3154
Conversation
@PranjalSahu awesome! The data structure should follow: https://insightsoftwareconsortium.github.io/itk-wasm/api/Mesh.html |
I'm slightly confused about why I've been requested as a reviewer, I don't do a whole lot with meshes (maybe will do more in the near future, but not a lot right now). Can you elaborate on what you were hoping for from me, @thewtex ? |
@GenevieveBuckley , no expectations, but since you did such an amazing job on the |
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.
Ok, I've had a look over it now. It seems like nice work overall.
I know it says above this is still a work in progress. Before it is merged, my two biggest suggestions are that:
- The
itkMeshSetGetCellTest.py
test assertions need to be strengthened, and - The dictionary returned on serialization currently includes a lot of empty string values, so it looks like there would be a fair bit of metadata loss upon serialization/de-serialization. This should also be addressed (and it will require a few new test assertions for these as well)
I don't feel like I can provide feedback on the header files, so I haven't left comments there.
elif key == 'direction': | ||
self.SetDirection(np.flip(value, axis=None)) | ||
else: | ||
self.GetMetaDataDictionary()[key] = value |
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.
There's no SetMetaDataDictionary
option?
Assuming that's the case, this is a nice workaround.
71591ef
to
5bfcf7b
Compare
5bfcf7b
to
d48ad1d
Compare
50aeb39
to
ded784d
Compare
@thewtex I found that we have Object name functionality present for all objects. I am thinking of using that instead of using a new field for name in the PointSet. If you agree then I will remove the addition of new name field from this PR. Also, I have not added the support for QuadEdge Mesh in the wasm data structure in this PR. |
Also I realized for code coverage I should instantiate all types of Cells while testing SetCellsAsArray. |
ded784d
to
798347a
Compare
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.
Mostly looks good.
f8252ed
to
29582eb
Compare
abfc813
to
2456a5b
Compare
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.
Looking great!!!
A few comments inline.
Could we also please add a test for serialization of a PointSet
?
aa5e580
to
4548adc
Compare
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.
LGTM, assuming CI passes.
@PranjalSahu could you please rebase on |
yes working on it. |
To investigate: Wrap PyVectorContainer for |
Set Method needed for better Python support and Mesh serialization. Already present GetPoints can be used in Python without any issues and therefore no additional method is needed.
Needed for better Python support and Mesh serialization using numpy.
@thewtex SetPointsArray is replaced with SetPoints. There are two versions for it now one The one with itkVectorContainerULF makes it easier to directly use NumPy arrays for setting I used the reinterpret_cast for this inside the newly added SetPoints: No need for assign operation is needed. Also, I found that for DynamicTraits it wasn't compiling as it Sample usage in Python:
|
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.
We have converged on an excellent outcome. @PranjalSahu thanks for your hard work. @GenevieveBuckley thanks for the review.
@PranjalSahu awesome to see the PointSet
serialization here, too!
|
||
|
||
# Check the serialization of mesh | ||
serialize_deserialize = pickle.loads(pickle.dumps(point_set)) |
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.
💯
{ | ||
itkExceptionMacro("Number of entries in given 1d array incompatible with the point dimension"); | ||
} | ||
auto * pointsPtr = reinterpret_cast<PointsContainer *>(points); |
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.
I'm sorry but this reinterpret_cast<PointsContainer *>(points)
very much looks like undefined behavior to me. (It tries to "reinterpret" the bits of an itk::VectorContainer
of scalars as an itk::VectorContainer
of itk::Point
.) How did you find out that it seemed to work at all?
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.
This can be tested by using a 1D array. I can write a small test case here if it is not present already.
I am checking if this is covered under test or not.
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.
Thanks @PranjalSahu I'm glad to see that you already tested this code:
ITK/Modules/Core/Common/test/itkPointSetTest.cxx
Lines 102 to 127 in 9a5dade
// Insert the points using SetPointArray. | |
PointsVectorContainerPointer pointsArray = PointsVectorContainer::New(); | |
// Test for in-compatible input array dimension | |
pointsArray->InsertElement(0, 1.0); | |
ITK_TRY_EXPECT_EXCEPTION(pset->SetPoints(pointsArray)); | |
pointsArray->Reserve(numOfPoints * pointDimension); | |
int index = 0; | |
for (int i = 0; i < numOfPoints; ++i) | |
{ | |
pointsArray->SetElement(index++, 1.0); | |
pointsArray->SetElement(index++, 2.0); | |
pointsArray->SetElement(index++, 3.0); | |
} | |
pset->SetPoints(pointsArray); | |
// Check the count of points after insertion using SetPoints. | |
pointsArrayAfter = pset->GetPoints(); | |
if (static_cast<int>(pointsArrayAfter->Size()) != numOfPoints) | |
{ | |
std::cerr << "Mismatch in count after inserting points." << std::endl; | |
return EXIT_FAILURE; | |
} |
Nevertheless, it's officially undefined behavior. Sorry 🤷. The C++ Standard does not guarantee that it works. In theory, it might do anything. And in practice, undefined behavior does lead to trouble. I just asked about a similar example at the ACCU mailing list, and Jonathan Wakely (@jwakely) confirmed that it is indeed undefined behavior.
So I would prefer to see the reinterpret_cast<PointsContainer *>
"trick" removed. But otherwise (at least), a note should be added that this function is unsafe.
for i in range(NumberOfCells): | ||
cells_array[i, :] = [i * 1, (i + 1) * 2, (i + 2) * 3] | ||
|
||
# Insert the cells in mesh by converting to 1D array |
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.
One similar test case is here which uses a 1D input array.
This PR adds the functionality of mesh serialization.
SetPoints method is added for 1D array input argument in PointSet. Used for numpy interface.
SetCellsArray and GetCellsArray are added in Mesh. To provider a simpler API for creating mesh having only one type of cell, additional SetCellsArray(cell_vector_container, itk.CommonEnums.CellGeometry) is added.
CreateCell private method is added that creates a new cell given its type. The function can be reused for other purposes. Future additions of new kinds of cells should add an entry here to support the Python focussed APIs.
For serialization, we follow the approach similar to the MeshFileWriter where first the cell type is passed, followed by the count of points in that cell and finally the point ids. The count of points is needed here as the itkPolygonCell can take a variable number of points.
The functions (SetPoints, SetCellsArray) take as argument 1D VectorContainer which can be easily converted to a numpy and reverse.
For pickle-based serialization, getstate and setstate methods are added in python (both for mesh and pointset)(refer ENH: pickle serialization for itk images (issue 1091) #2829).
Support for key based updation of points, cells, point data, and cell data in the mesh is also added.
Test for consistency of points and cells using the newly added functions is added. The newly added API should perform seamlessly with older APIs and the results should be consistent.
Python Tests have also been added for mesh serialization, point set serialization, dict based methods in python, and for the conversion from VTK Mesh to ITK Mesh.
PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.