Correctness: opt-in deterministic conv, pruning empty-mask guard, TensorField backprop regression test - #17
Merged
Merged
Conversation
… backprop test Three correctness items for MinkowskiEngine convolution/pruning/tensor-field. 1. Opt-in deterministic convolution (upstream NVIDIA#554, NVIDIA#504) Conv output depends on input coordinate ordering (atomicAdd accumulation + kernel-map dispatch order), so repeated runs on the same unsorted input differ in low-order bits, worst at stride>1 / transpose. Add an opt-in switch that canonicalizes coordinate order before the backend call: - MinkowskiEngine/utils/determinism.py: sorted_coordinates(sparse_tensor) returns a new SparseTensor with rows in lexicographic coordinate order (stable per-column radix argsort, overflow-free, handles arbitrary D and negative coords), plus a process-wide set_deterministic/is_deterministic flag. - MinkowskiConvolution / MinkowskiConvolutionTranspose forward sort their input through the helper when the flag is on (skipped for the use_mm kernel_volume==1 path). Default behavior unchanged; documented perf cost of one sort per conv. - Exported as ME.set_deterministic / ME.is_deterministic / ME.sorted_coordinates. 2. Pruning empty-mask guard (upstream NVIDIA#579) Reproduced on this fork: an all-False mask through MinkowskiPruning on CUDA already returns a valid empty SparseTensor; downstream conv and backward run cleanly with finite (empty) grads (src/pruning_gpu.cu handles the size-0 case). No code change needed; add a regression test that locks in the behavior. 3. TensorField backprop regression test (upstream NVIDIA#395) The merge_sort illegal-address crash in inverse_mapping backward was fixed upstream by 02fc608 (in our history). Add a field -> sparse -> conv -> slice -> field backward test over several iterations with varying N (~50k, D=3, batch 2) asserting finite gradients. Tests are CUDA-gated (skipUnless) and skip cleanly on CPU CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three correctness items in the Python layer plus regression tests. No
src/*.cuchanges; existing built backend is unchanged.1. Opt-in deterministic convolution (upstream #554, #504)
Convolution output depends on the input coordinate ordering:
atomicAddaccumulation and kernel-map dispatch order mean that running the same conv on the same point set in a different row order yields low-order-bit differences, worst atstride > 1and transpose. The community-confirmed workaround is to sort coordinates before the conv.MinkowskiEngine/utils/determinism.py:sorted_coordinates(sparse_tensor)returns a newSparseTensorwith rows in canonical lexicographic coordinate order. Implemented as a stable per-column radixargsort(batch column most significant) so it never packs coords into a single integer rank (overflow-free), and it is correct for arbitrary spatial dimensionDand negative coordinates.set_deterministic(mode)/is_deterministic()flag (off by default).MinkowskiConvolution/MinkowskiConvolutionTransposeforwardroute the input throughsorted_coordinateswhen the flag is on (skipped for theuse_mm,kernel_volume == 1, path which is a plain matmul). Documented cost: one coordinate sort per conv; intended for reproducibility/debugging, not throughput.ME.set_deterministic,ME.is_deterministic,ME.sorted_coordinates.Default behavior is unchanged (opt-in only).
2. Pruning empty-mask guard (upstream #579)
Reproduced on this fork first: an all-False mask through
MinkowskiPruningon CUDA followed by a downstream conv and backward. It does not crash. The CUDA pruning kernel already emits a valid emptySparseTensorfor the size-0 case, downstream conv produces zero-size outputs, and backward yields finite (empty, zero) gradients. Per the plan, no code change was needed; added a regression test that locks in the behavior.3. TensorField backprop regression test (upstream #395)
The reported
merge_sortcudaErrorIllegalAddressininverse_mappingbackward was fixed upstream by commit02fc608, which is in this fork's history. Added afield -> sparse -> conv -> slice -> fieldbackward test (~50k points, D=3, batch 2) run over 5 iterations with varying N to catch intermittence; asserts finite gradients. Passes (no xfail required).Tests
New CUDA-gated modules (skip cleanly on CPU-only CI):
tests/python/determinism.pytests/python/pruning_empty.pytests/python/tensorfield_backward.pyVerified on RTX 5090 / CUDA 12.8:
convolution.py(minusTestPCD),pruning.py,half_precision.py: 26 passed, 4 skipped, 2 deselected.