-
-
Notifications
You must be signed in to change notification settings - Fork 668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: Add TransformMINC to default modules #4160
Conversation
I assume you will be looking into test failures? |
Surprised to see this as I already use this feature regularly and it works for me. Investigating. Edit: Thankfully this is a feature component I don't use regularly |
This is error:
Paging @vfonov while I look at the code. |
Ah ha, this looks like a typo in the test expected output value, v2[1] is uninitalized:
|
Re-ping @vfonov |
/azp run ITK.Windows |
Looks like the problem is in the allocation of the 4x4 matrix by
This should be enough so that the maintainers of the |
Maintainers looked and it's not clear why it would not allocate enough memory here and not in 1000 other places where it's used. Maybe it's corrupted memory from somewhere else? |
A few more thoughts at this:
Not sure how conclusive the above are, as @vfonov might have a point when saying that a previous memory problem may only appear at a latter stage, but they may provide some useful information. |
Could it be possible to compile and run tests in windows with Address Sanitizer? |
Another thought:
Not sure if this has an impact on the bug, but should all factories be registered only once in the |
Rebase on |
Rebased. |
00c0eb0
to
c3cd3a5
Compare
@jhlegarreta @dzenanz #4175 broke the tests completely. |
Do the tests pass if #4175 is reverted? |
No. The module was not active and affected tests were not executed in PR #4175. ITK compiles correctly on all 3 platforms, without warnings (look for "default-minc"): https://open.cdash.org/index.php?project=Insight&date=2023-09-19. Tests do not pass, as it happened with the first commit pushed to this branch; test failures were then detected, and should be the same ones that have appeared with the latest push force.
@gdevenyi No, look at the files that you modified in your last push force: the rebase was not done correctly, and the fixes to the test were not carried over correctly. You can look for the changes in the git commits on the GitHub interface ("(...) force-pushed the default-minc branch (...) times, most recently from (...) to (...)"), e.g. 00c0eb0, or doing a PR #4175 only changed the style: as the lines modified overlap with the fixes to the test in the commits in this branch, rebasing needed manual work. Windows tests will still not pass after a proper rebase, unless the bug discussed with Vladimir is not stable. |
That was mentioned in #4175 (comment).
See #4175 (comment) and #4175 (comment). As said, changes can easily be recovered investigating commits, and should not be hard. |
Before, the tests were failing with an error in the result. After #4175, the tests don't run at all due to C++ code issues I don't understand.
|
Gabriel, the test is running: The trace that you post are mainly exceptions that are captured correctly ("Caught expected exception") and other print statements; you just need to include back #4160 (comment) and 00c0eb0. That will make Linux and macOS pass, and we will be left back with the Windows segfault issue. |
So, #4175 extensively changed the code such that a simple rebase failed, and massively raised the noise level on the test such that I couldn't see what was happening. Fine. |
99052cf
to
647e132
Compare
Has been said multiple times: code is read many more times that it is written, and readability and consistency are important. Printing images or other ITK objects is something that I never (or very, very rarely) do in tests, but there are many tests that have such statements; I usually do not remove them, e.g. And that raises any noise level, and frequently makes the error trace to be clipped on the dashboard.
That is just the exception macro. |
@@ -19,5 +19,4 @@ itk_module(ITKIOTransformMINC | |||
TransformIO::MINC | |||
DESCRIPTION | |||
"${DOCUMENTATION}" | |||
EXCLUDE_FROM_DEFAULT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot enable this by default until Windows segfault is fixed.
@@ -628,7 +628,7 @@ check_composite2(const char * transform_file, const char * transform_grid_file) | |||
pnt[0] = 1.0; | |||
pnt[1] = pnt[2] = 0.0; | |||
// expected transform: shift by 1 , rotate by 45 deg | |||
v2[0] = v[1] = sqrt(2); | |||
v2[0] = v2[1] = sqrt(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless someone is going to look into Windows issue soon, we could turn this fix into a new PR that can be merged quickly.
I can't fix windows things. |
Add TransformMINC to default modules.
MINC is already enabled by default but it's transforms were not.
Ref: conda-forge/libitk-feedstock#83