ENH: Add TractographyTRX module#5925
Conversation
551c30b to
5c67a38
Compare
|
Please squash commits and force-push. |
c3dc597 to
245b666
Compare
thewtex
left a comment
There was a problem hiding this comment.
@mattcieslak thanks!
One request inline.
| #-- | ||
| #-- ## Compliance Level 4 star (Very high-quality code, perhaps small community dependance) | ||
| #-- - [X] Meets all ITK code style standards | ||
| #-- - [ ] No external requirements beyond those needed by ITK proper // question - libzip and trx-cpp are added via cmake. do these count as external dependencies? |
There was a problem hiding this comment.
As you are using FetchContent, I think that counts as no external dependencies.
As zlib is bundled with ITK (and cannot be disabled), why do you have this section?
There was a problem hiding this comment.
I had a hard time getting libzip (a trx-cpp dependency) compiled with the built in zlib. This is clunky but eventually got the CI tests passing on all the platforms.
IIRC there were was an issue where tthe header path changed between ITK versions (src/zlib.h to src/itkzlib-ng/zlib.h) and an issue with where windows was putting the library
|
@thewtex You did not post the in-line request 😄 |
Added :-) |
|
I tried this locally, and ran into: VS projects were still generated. Trying to build resulted in: |
There is an open PR to refactor ITK's embedded Eigen to conform with existing pattern in ITK. Usage such as the include path may change soon. This looks like similar usage to the TotalVariations remove module where there is a FetchConent project that also needs to use Eigen. |
245b666 to
43f49d7
Compare
|
@dzenanz I updated the build based on TotalVariation's, could you please try again? I also set the shasum to the updated commit. |
|
Hello, I'm not the sure of the full implication for this module but there are a couple PR related. An update to they Eigen3 library handled in ITK is here: #5831 I'm not sure how much of a difference it will make with your remote, but it removes the need for a lot of special handling of the Eigen library in the CMake code. And an update an clean update of the CMake code for TotalVariations: InsightSoftwareConsortium/ITKTotalVariation#57 But this does other simiplifications and updates to get the module working smoothly with ITKv6. I'm going to work on the Eigen library update today hopefully the results will allow some simplification with your usage, |
|
It now builds and tests pass. Wrapping generates an error, but that is expected given its complete absence: I would imagine it is not hard to write those wrapping specifications, as there are not a lot of classes, and they don't have a lot of template parameters either. But I assume you already have it on your TODO list? |
thewtex
left a comment
There was a problem hiding this comment.
Thanks!
Please create new PRs to update the GIT_TAG as improvements land.
This PR adds a TractographyTRX remote module for reading/writing/interacting with streamline data in the TRX format. It follows up on this discussion.
We'd be happy for feedback/suggestions on the module, or help getting it merge ready. Thanks in advance!
PR Checklist