Allow tractograms with large offsets, 2 - #59
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It introduces several correctness/reliability risks around large-count JSON conversion and large-file allocation/testing that can cause silent truncation or OOM/failing tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates TRX count/shape handling to support tractograms whose vertex/offset counts exceed the 32-bit signed integer range (Issue #50), aligning trx-cpp behavior with large datasets generated by tools like trx-python.
Changes:
- Switched vertex/streamline count plumbing (and Eigen map shapes) from
intto 64-bit (long long) across load/save and helpers. - Added JSON numeric workaround
_json_int64()and began writing large counts intoheader.jsonasdouble(json11 limitation). - Added a regression test that builds a minimal on-disk TRX directory with
NB_VERTICES > INT32_MAXand uint64 offsets.
File summaries
| File | Description |
|---|---|
tests/test_trx_large.cpp |
Adds a regression test for loading NB_VERTICES > INT32_MAX via sparse file setup. |
tests/CMakeLists.txt |
Registers the new test_large binary with CTest/GTest discovery. |
src/trx.cpp |
Extends count parsing and ZIP/directory loading paths to 64-bit counts and JSON number handling; updates merge header writing. |
include/trx/trx.tpp |
Propagates 64-bit sizing through templates (resize, remap, normalize_for_save, subset, finalize, etc.). |
include/trx/trx.h |
Introduces _json_int64() helper and updates public types/signatures to accept 64-bit counts/shapes. |
include/trx/detail/dtype_helpers.h |
Updates remap() and _compute_lengths() interfaces to accept 64-bit dimensions/counts. |
Review details
Suppressed comments (1)
tests/test_trx_large.cpp:37
- fs::remove_all(dir) uses the throwing overload; if cleanup fails (permissions, concurrent access) the test can fail after the assertions have passed. Prefer the std::error_code overload (or rely on the suggested RAII guard) to make cleanup best-effort.
fs::remove_all(dir);
- Files reviewed: 6/6 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| mio::shared_mmap_sink _create_memmap(std::string filename, | ||
| const std::tuple<int, int> &shape, | ||
| const std::tuple<long long, long long> &shape, | ||
| const std::string &mode, | ||
| const std::string &dtype, | ||
| long long offset) { |
| inline long long _json_int64(const json &value) { | ||
| return static_cast<long long>(value.number_value()); | ||
| } |
|
Looks good overall. The copilot suggestions seem pretty reasonable, no? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
+ Coverage 83.62% 83.66% +0.04%
==========================================
Files 18 19 +1
Lines 9195 9227 +32
Branches 1328 1329 +1
==========================================
+ Hits 7689 7720 +31
- Misses 1505 1506 +1
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The first one may be true, I am not sure, I don't really think it has much to do with this PR. I only have to read large TRX files currently, not write them, so I have not tried and seen if what copilot is saying is true there. I think the third one seems excessive to me. The last two also seem excessive, but I can rewrite the last test to use the same functions the other TRX tests use. |
|
Fair enough. @frheault : any chance you could take another look before we merge? |
|
I believe the proposed change should be alright for read, but if you say you did not test/check the write, I believe it should be validated that loading a large TRX can be saved without losing data/payload. For example if you a very large TRX (1 billion streamlines) and then subsample a large chunk (100 million streamlines) I would expect that it could be saved if we can load it. Also, is this already working in python (or another language)? |
|
This is already working in Python, so I can check it against that |
Fixes #50
Mostly updates things that handle the number of vertices to use long long instead of int. This allows vertex count to be greater than 2^31, which I believe the Python implementation of TRX already handles. Also, of note, doubles are used to save integers to JSON because JSON 11 does not handle int64 but does handle doubles for some reason. I also added a short, quick test using sparse files to validate it.