ENH: KernelTransform's PointSet should use minimal point data #4526
Conversation
This data is not used, so it is better to have a lighter-weight char, instead of the heavier PointType there. Having PointType there might also confuse someone into thinking that point coordinates should be provided there, or might be retrieved from it.
github-actions
bot
added
type:Enhancement
| #ifdef ITK_FUTURE_LEGACY_REMOVE | ||
| using PointSetType = PointSet<unsigned char, VDimension, PointSetTraitsType>; | ||
| #else | ||
| using PointSetType = PointSet<InputPointType, VDimension, PointSetTraitsType>; | ||
| #endif |
There was a problem hiding this comment.
Ideally you would use PointSet<void, VDimension, PointSetTraitsType>, right?
For what it's worth, I noticed that sizeof(PointSet<unsigned char>) == sizeof(PointSet<long long>), so for the size it does not really seem to matter.
Do you have an idea why KernelTransform doesn't simply use an std::vector of points?
There was a problem hiding this comment.
PointSet<void, VDimension, PointSetTraitsType> does not compile, unfortunately. That was my first idea 😄
Do you have an idea why KernelTransform doesn't simply use an std::vector of points?
Maybe to make it more convenient to cooperate with meshes?
There was a problem hiding this comment.
sizeof(PointSet<>) is unaffected, as it is independent of data associated with each point.
There was a problem hiding this comment.
Maybe to make it more convenient to cooperate with meshes?
Possible 🤔 But then, would your change break old use cases of cooperation with meshes of the original "point data" type?
I'm not opposed to this PR, I'm just curious. I see that that "point data type" can be problematic.
There was a problem hiding this comment.
Before this PR, there were two places for PointType (i.e. point coordinates). One which was envisioned for it (in PointsContainer) and another one in PointDataContainer, which was envisioned for other things (such as point color, intensity, importance, whatever). This PR changes PointData to be an unsigned char.
There was a problem hiding this comment.
sizeof(PointSet) == sizeof(PointSet), so for the size it does not really seem to matter.
This does seem potentially disruptive without convincing benefit if there is not a size difference and the point data is not allocated. I am not sure what unsigned char would be used for. I could see a use case for putting the transform points in the PointData.
There was a problem hiding this comment.
If I understand correctly, KernelTransform ignores the "associated data" of the PointSet. So ideally, it should support any type of PointSet, independent of its "associated data". Right? Just asking, for my understanding.
There was a problem hiding this comment.
If there is a use case for supporting any type with the Kernel Transform, then the point set or its pixel type would need to be a template parameter. But I am not seeing that use case, at least in any tests in this patch. And the change would not be to always unsigned char.
This data is not used, so it is better to have a lighter-weight char, instead of the heavier
PointTypethere. HavingPointTypethere might also confuse someone into thinking that point coordinates should be provided there, or might be retrieved from it.PR Checklist