This PR addresses issue #32 regarding failing DCHECKs in the Morton encoder.
Type of change
[x] Bug fix (non-breaking change which fixes an issue)
Detailed summary
Wavemap's implementation of Morton encoding does not support negative indices. Specifically, the conversion (Index -> Morton -> Index) is reversible for positive coefficients, but for negative coefficients, their truncated two's complement is returned instead.
The DCHECK that failed was there to alert users when a negative index is Morton encoded since I originally thought this non-reversible conversion would only be requested by accident. However, most of wavemap's code only performs one-way conversions (Index -> Morton) and uses the Morton numbers to compute relative offsets. These are valid regardless of the sign and not problematic. It would be possible to update wavemap's code to only Morton encode positive indices, but this would often require back-and-forth conversions that make the code harder to read and slightly less efficient.
The changes in this PR:
make the sign checks during Morton encoding optional
add inline documentation
enable DCHECK_ALWAYS_ON in CI
Fixes #32
Testing
The changes were tested through the regular unit tests and by manually running the new code on the demo datasets.
Checklist:
[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my code
[x] I have commented my code, particularly in hard-to-understand areas
[x] I have made corresponding changes to the documentation
[x] Any required changes in dependencies have been committed and pushed
Description
This PR addresses issue #32 regarding failing DCHECKs in the Morton encoder.
Type of change
Detailed summary
Wavemap's implementation of Morton encoding does not support negative indices. Specifically, the conversion (Index -> Morton -> Index) is reversible for positive coefficients, but for negative coefficients, their truncated two's complement is returned instead.
The DCHECK that failed was there to alert users when a negative index is Morton encoded since I originally thought this non-reversible conversion would only be requested by accident. However, most of wavemap's code only performs one-way conversions (Index -> Morton) and uses the Morton numbers to compute relative offsets. These are valid regardless of the sign and not problematic. It would be possible to update wavemap's code to only Morton encode positive indices, but this would often require back-and-forth conversions that make the code harder to read and slightly less efficient.
The changes in this PR:
DCHECK_ALWAYS_ON
in CIFixes #32
Testing
The changes were tested through the regular unit tests and by manually running the new code on the demo datasets.
Checklist: