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.

Sign up for GitHub

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: KernelTransform's PointSet should use minimal point data #4526

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ENH: KernelTransform's PointSet should use minimal point data #4526

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Modules/Core/Transform/include/itkKernelTransform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,11 @@ class ITK_TEMPLATE_EXPORT KernelTransform : public Transform<TParametersValueTyp
* specifically, the source and target landmark lists. */
using PointSetTraitsType =
DefaultStaticMeshTraits<TParametersValueType, VDimension, VDimension, TParametersValueType, TParametersValueType>;
#ifdef ITK_FUTURE_LEGACY_REMOVE
using PointSetType = PointSet<unsigned char, VDimension, PointSetTraitsType>;
#else
using PointSetType = PointSet<InputPointType, VDimension, PointSetTraitsType>;
#endif
Comment on lines +119 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

sizeof(PointSet<>) is unaffected, as it is independent of data associated with each point.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly!

Copy link
Member

Choose a reason for hiding this comment

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

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.


using PointSetPointer = typename PointSetType::Pointer;
using PointsContainer = typename PointSetType::PointsContainer;
Expand Down
Loading