Reject non-invertible volume affines

[yinkev]

Closes #440.

Summary

• make scaled-orthogonality validation independent of column scale
• reject zero or non-finite scales and dependent directions in the shared _is_matrix_orthogonal() helper
• add direct helper coverage plus regressions for both Volume and VolumeGeometry
• preserve valid tiny independent scales

Root cause

When require_unit=False, _is_matrix_orthogonal() compared the unnormalized Gram matrix against the diagonal matrix of squared column norms using an absolute tolerance. A zero column passed exactly, while a sufficiently small dependent column passed because its off-diagonal dot product fell below that tolerance.

The fix normalizes each nonzero finite column before comparing directions. This removes the scale dependence without adding separate rank or inverse checks in _VolumeBase.

Validation

• regression against upstream/master: 4 failed, 3 passed
• focused regression after the fix: 7 passed
• tests/test_spatial.py and tests/test_volume.py: 252 passed
• full test suite: 2,033 passed, 153 expected optional-dependency skips
• repository-wide Flake8
• git diff --check

[CPBridge]

Hi @yinkev I am not really sure I understand the problem here. Can you help me understand it? It has been a while since I learnt linear algebra, so I might be wrong in parts of the following.

I understand that the scale factors should be finite, and that is not currently checked for. However, I think a scale factor of 0.0 in any column would be caught by the current orthogonality check anyway so a further check for zero scale factors seems unnecessary (am I wrong about this?). This leaves only the infinite scale factor case, which is an unlikely situation to encounter in real code, though it may make sense to add a check for it. But couldn't it more easily be checked for by checking that all elements in the input matrix are finite?

The rank-deficient part escapes me. I'm fairly sure a rank-deficient square matrix is always non-orthogonal, and we are already testing the matrix for non-orthogonality. If that test is functioning correctly, I don't see any need to add a further test for rank deficiency. Are you saying that the existing non-orthogonality test (in highdicom.spatial._is_matrix_orthogonal() ) is insufficient in some rank-deficient cases? If so, why not change that function rather than add further code around its invocation in the _VolumeBase constructor? I also tried the example that you added in the tests, and it seems that the existing highdicom.spatial._is_matrix_orthogonal() catches this already, which suggests that the new check is unnecessary. Note that despite the various mentions of deficiencies of np.linalg.inv in the comments in the tests and above on this PR, that function is not actually used in the current checks. Can you point me to an example matrix that is rank deficient but is determined to be orthogonal by the current implementation of highdicom.spatial._is_matrix_orthogonal()?

What am I missing?

[yinkev]

I went back through the exact call path. You're right that the fix belongs in _is_matrix_orthogonal(), and I overcomplicated the PR.

The key detail is that _VolumeBase calls it with require_unit=False. In that mode, a zero column passes, and a sufficiently small dependent column can also pass because the off-diagonal dot product falls below the absolute tolerance. So the underlying bug is real, but the current patch is not the clean fix.

I agree the extra rank and inverse checks around _VolumeBase are unnecessary. I'll simplify this by fixing the scale-dependent check in _is_matrix_orthogonal() and updating the tests around that.

Thanks for catching the design issue.

[yinkev]

Updated this to fix _is_matrix_orthogonal() directly and removed the extra rank and inverse checks from _VolumeBase. The check now compares normalized column directions, so zero and small dependent columns are rejected while tiny independent scales still work. I also removed the LAPACK-dependent test. This is ready for review.

[CPBridge]

Thank you @yinkev for your help finding and fixing this and the other recent bugs

I will put out a patch release (v0.28.1) with all these fixes