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

Mesh volume and diameters #427

Merged
merged 5 commits into from Sep 28, 2018
Merged

Mesh volume and diameters #427

merged 5 commits into from Sep 28, 2018

Conversation

JoostJM
Copy link
Collaborator

@JoostJM JoostJM commented Sep 20, 2018

To increase consistency between surface area, volume and maximum diameters, calculate all 3 on the same triangle mesh generated using the marching cubes algorithm.

Original definition of volume (No of voxels * volume of 1 voxel) is retained as ApproximateVolume.

This also matches the definition of shape features as they are defined by IBSI.

cc @Radiomics/developers

@fedorov
Copy link
Collaborator

fedorov commented Sep 25, 2018

Since both surface areas are approximations, how about renaming them to MeshSurfaceArea and VoxelSurfaceArea?

@fedorov
Copy link
Collaborator

fedorov commented Sep 25, 2018

Or maybe even better - TriangulatedSurfaceArea.

"""
if self.diameters is None:
self.diameters = self.calculateDiameters()
return self.diameters[2]

def getMajorAxisFeatureValue(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be MajorAxisLength? To me, "major axis" implies vector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fedorov, good point! But I'll change that in a separate PR, as it is unrelated to the changes proposed here.

@JoostJM
Copy link
Collaborator Author

JoostJM commented Sep 25, 2018

Since both surface areas are approximations, how about renaming them to MeshSurfaceArea and VoxelSurfaceArea?

@fedorov, certainly a good suggestion (although I propose we do this for the 2 volume features, as there is only 1 surface area feature, the mesh-based one. i.e. MeshVolume and VoxelVolume. Or maybe VoxelbasedVolume / VoxelwiseVolume?

Or maybe even better - TriangulatedSurfaceArea.

Certainly also a possibility, although I'm more partial to Mesh, Triangulated makes me think of triangulation. Plus, MeshVolume is easier to pronounce... @pieper, @hugoaerts, any preferences?

@JoostJM
Copy link
Collaborator Author

JoostJM commented Sep 25, 2018

@fedorov, do you also want me to change the names of Maximum3DDiameter to Maximum3DMeshDiameter, or maybe MeshDiameter3D (with 3D replaced by 2DColumn, 2DRow and 2DSlice for the others?

@pieper
Copy link
Contributor

pieper commented Sep 26, 2018

I like the idea of prefixing the measurement with Mesh or Voxel (e.g. MeshVolume, MeshSurfaceArea and VoxelVolume and VolumeSurfaceArea) for clarity and consistency. Same logic would imply MeshMaximum3DDiameter, and other metrics.

@fedorov
Copy link
Collaborator

fedorov commented Sep 26, 2018

This sounds fine to me. I suggested Triangulation in place of Mesh because to be precise - voxel surface is also a mesh. But I agree Triangulation is too long, and the distinction can be explained in the documentation.

@JoostJM
Copy link
Collaborator Author

JoostJM commented Sep 27, 2018

@fedorov, @pieper, changing all feature names to explicitly state they are Mesh or Volume really lengthens the names. Therefore, I have for now just updated the documentation. There it is now specified that all shape features, unless otherwise specified, are based on the mesh approximation.

Only exceptions are VoxelVolume (which I did explicitly rename, to contrast it with MeshVolume) and the PCA-derived features (major, minor and least axis lengths, elongation and flatness), but for those I explicitly mention they are not based on the mesh.

@pieper
Copy link
Contributor

pieper commented Sep 27, 2018

@JoostJM yes, it makes sense to have the human readable names short and easy to use.

@fedorov when you express these in SR, are there modifiers to differentiate the 'from mesh' vs the 'from voxel' variants? For consistency probably the 'from voxel' modifier should be used for the cases Joost mentioned even if there isn't a 'from mesh' variant just so consumers of the document can know without referring to the documentation.

@fedorov
Copy link
Collaborator

fedorov commented Sep 27, 2018

I think it is ok to not have that modifier explicitly in the name. We can't expect deriving fully qualified definition of the feature calculation from just the name in the general case. We can handle mapping as needed for SR encoding via a separate lookup table or some decorators in the function documentation.

In the case where PyWavelet needs to be compiled, numpy must be available.
Even though it is specified in the requirements, it will not be available for compiling PyWavelets if not previously installed.
Prevent failure of installation by explicitly checking if numpy is available and install it if not.
In concordance with the IBSI definition of volume, calculate it using the triangle mesh that is also generated for the calculation of surface area.
This yields a better correspondence between the calculated volume and surface area, needed for calculation of several subsequent shape features (e.g. sphericity).

Original formula for calculation is retained as 'new' feature "ApproximateVolume".
Similar to the change in volume calculation, change the calculation for maximum diameters into one based on the triangle mesh generated by the marching cubes algorithm. This yields a higher consistency between volume, surface area and diameters, as they are now all based on the same volume (defined by the triangle mesh).

Additionally, this is also computationally more efficient, as only one pass over all voxels is required, instead of 1 for surface area and volume, and another one for the calculation of the maximum diameters.
To better reflect computation method, explicitly name the two volume features `MeshVolume` and `VoxelVolume`.
Add documentation specifying the unless otherwise mentioned, features are based on the mesh approximation of the shape.

Only exceptions to this are the `VoxelVolume` and PCA-derived features (`MajorAxisLength`, `MinorAxisLength`,`LeastAxisLength`, `Elongation` and `Flatness`. For these features, it is explicitly stated in the documentation that these do not make use of the mesh approximation, but the segmented voxel(center)s themselves.
@JoostJM JoostJM merged commit 14809a5 into AIM-Harvard:master Sep 28, 2018
@JoostJM JoostJM deleted the mesh-volume branch September 28, 2018 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants