ENH: Replace InitializeElastixLibrary() by InitializeElastixExecutable() #248
Conversation
There was a problem hiding this comment.
Looks good, but I think you also need to call InitializeElastixExectuable() in the transformix executable (Core/Main/transformix.cxx).
I think you're right, thanks, I'll amend the commit, but do you think it would matter? Would it really change the behavior of the transformix executable? It depends on where, on which places IsElastixLibrary() is actually checked: 64ced59
And is there any reason to perform the assert in the destructor of the ELASTIX class?
Well... IsElastixLibrary() should always return true, from the begin of the lifetime of the ELASTIX library object right until the very end of its lifetime. So I thought it would be nice to assert so both at the begin and at the end of its lifetime. If it really bothers you, I can remove the assert, no problem, but otherwise I think I just leave it in.
Kasper Marstal reported yesterday via direct messaging that the introduction of `IsElastixLibrary()`, pull request #231, commit 64ced59 (March 2, 2020, "Replace _ELASTIX_BUILD_LIBRARY by BaseComponent::IsElastixLibrary"), did break SimpleElastix. SimpleElastix depends on the library interface, while it does not construct an `elastix::ELASTIX` library object. The most elegant and reliable fix appears to be flipping the default value returned by `IsElastixLibrary()`, from `false` to `true`, and then ensuring that the value is set to `false` at the start of each run of the executable. This applies to the transformix executable as well.
N-Dekker
force-pushed
the
Flip-default-from-executable-to-library
branch
from
0b74030 to
ef64fb2
Compare
|
@kaspermarstal If you think the pull request is fine (and if it compiles and passes the tests), you may just click "Rebase and merge", to get rid of the blocker 😃 |
The change broke the tests with the transformix executable so it was definitely needed. Thanks! |
Thanks for pressing the button, Kasper! 👍 Minor comment: it appears that your merge has resulted in two commits, instead of one, at https://github.com/SuperElastix/elastix/commits/develop I think that's because you clicked "Merge pull request", right? When you would have clicked "Rebase and merge", I think it would have resulted in a single commit the develop branch, combining the code change and the merge into one. Which is my personal preference. No need to change it for this time, of course! 😃 |
Removed obsolete `elastix::BaseComponent::InitializeElastixLibrary()` calls, as Elastix is now initialized as a library by default: SuperElastix/elastix#248
Removed obsolete `elastix::BaseComponent::InitializeElastixLibrary()` calls, as Elastix is now initialized as a library by default: SuperElastix/elastix#248
Removed obsolete `elastix::BaseComponent::InitializeElastixLibrary()` calls, as Elastix is now initialized as a library by default: "ENH: Replace InitializeElastixLibrary() by InitializeElastixExecutable()" SuperElastix/elastix#248 SuperElastix/elastix@ef64fb2 Adjusted CMake `Elastix_LIBRARIES`, as the Elastix library CMake targets are now named elastix_lib and transformix_lib: "ENH: Allow building exe and lib of elastix and transformix together" SuperElastix/elastix#232 SuperElastix/elastix@3944a13
Removed obsolete `elastix::BaseComponent::InitializeElastixLibrary()` calls, as Elastix is now initialized as a library by default: "ENH: Replace InitializeElastixLibrary() by InitializeElastixExecutable()" SuperElastix/elastix#248 SuperElastix/elastix@ef64fb2 Adjusted CMake `Elastix_LIBRARIES`, as the Elastix library CMake targets are now named elastix_lib and transformix_lib: "ENH: Allow building exe and lib of elastix and transformix together" SuperElastix/elastix#232 SuperElastix/elastix@3944a13
Removed obsolete `elastix::BaseComponent::InitializeElastixLibrary()` calls, as Elastix is now initialized as a library by default: "ENH: Replace InitializeElastixLibrary() by InitializeElastixExecutable()" SuperElastix/elastix#248 SuperElastix/elastix@ef64fb2 Adjusted CMake `Elastix_LIBRARIES`, as the Elastix library CMake targets are now named elastix_lib and transformix_lib: "ENH: Allow building exe and lib of elastix and transformix together" SuperElastix/elastix#232 SuperElastix/elastix@3944a13
Removed obsolete `elastix::BaseComponent::InitializeElastixLibrary()` calls, as Elastix is now initialized as a library by default: "ENH: Replace InitializeElastixLibrary() by InitializeElastixExecutable()" SuperElastix/elastix#248 SuperElastix/elastix@ef64fb2 Adjusted CMake `Elastix_LIBRARIES`, as the Elastix library CMake targets are now named elastix_lib and transformix_lib: "ENH: Allow building exe and lib of elastix and transformix together" SuperElastix/elastix#232 SuperElastix/elastix@3944a13
Removed obsolete `elastix::BaseComponent::InitializeElastixLibrary()` calls, as Elastix is now initialized as a library by default: "ENH: Replace InitializeElastixLibrary() by InitializeElastixExecutable()" SuperElastix/elastix#248 SuperElastix/elastix@ef64fb2 Adjusted CMake `Elastix_LIBRARIES`, as the Elastix library CMake targets are now named elastix_lib and transformix_lib: "ENH: Allow building exe and lib of elastix and transformix together" SuperElastix/elastix#232 SuperElastix/elastix@3944a13
Removed obsolete `elastix::BaseComponent::InitializeElastixLibrary()` calls, as Elastix is now initialized as a library by default: "ENH: Replace InitializeElastixLibrary() by InitializeElastixExecutable()" SuperElastix/elastix#248 SuperElastix/elastix@ef64fb2 Adjusted CMake `Elastix_LIBRARIES`, as the Elastix library CMake targets are now named elastix_lib and transformix_lib: "ENH: Allow building exe and lib of elastix and transformix together" SuperElastix/elastix#232 SuperElastix/elastix@3944a13
Removed obsolete `elastix::BaseComponent::InitializeElastixLibrary()` calls, as Elastix is now initialized as a library by default: "ENH: Replace InitializeElastixLibrary() by InitializeElastixExecutable()" SuperElastix/elastix#248 SuperElastix/elastix@ef64fb2 Adjusted CMake `Elastix_LIBRARIES`, as the Elastix library CMake targets are now named elastix_lib and transformix_lib: "ENH: Allow building exe and lib of elastix and transformix together" SuperElastix/elastix#232 SuperElastix/elastix@3944a13
Kasper Marstal reported yesterday via direct messaging that the introduction of
IsElastixLibrary(), pull request #231, commit 64ced59 (March 2, 2020, "Replace _ELASTIX_BUILD_LIBRARY by BaseComponent::IsElastixLibrary"), did break SimpleElastix. SimpleElastix depends on the library interface, while it does not construct anelastix::ELASTIXlibrary object.The most elegant and reliable fix appears to be flipping the default value returned by
IsElastixLibrary(), fromfalsetotrue, and then ensuring that the value is set tofalseat the start of each run of the executable.