-
Notifications
You must be signed in to change notification settings - Fork 175
Description
I've been trying to run KPConv into a custom pipeline and ran into a critical issue that I think deserves attention.
The repo uses -1 as a padding flag for missing neighbors, great I use it as flag too. However, the handling of these -1 indices is only implemented in the deformable branch of the KPConv.forward(), where they get redirected to a shadow point.
In the non-deformable case, the forward() uses neighb_inds directly in operations like gather() without any filtering or redirection. If -1 is present (which can happen even with calibration), this leads to a CUDA crash due to out-of-bounds indexing.
Architectures typically mix deformable and non-deformable blocks.
Calibration (as implemented in the repo) only aims to cover ~90% of cases, so padding with -1 is still expected.
There's no guarantee that neighb_inds will be clean in non-deformable blocks.
The crash is silent until the model hits a gather() or matmul with invalid indices.
What I did to patch it:
I added a patch in the non-deformable branch of KPConv.forward() that:
python
Add shadow point
s_pts = torch.cat((s_pts, torch.zeros_like(s_pts[:1, :]) + 1e6), 0)
x = torch.cat((x, torch.zeros_like(x[:1, :])), 0)
Redirect -1 to shadow point
neighb_inds = neighb_inds.clone()
neighb_inds[neighb_inds == -1] = s_pts.shape[0] - 1
This mirrors the logic used in the deformable path and prevents the crash.
Suggestion:
Either:
Apply this redirection logic in both deformable and non-deformable paths.
Or enforce that neighb_inds never contains -1 (which would require stricter calibration or filtering upstream).
But as it stands, the system accepts -1 as a valid padding flag, yet doesn't consistently handle it which feels like a design inconsistency and a potential trap for users.
I may be wrong but at least this is how I see it :D
cheers!