Skip to content
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

ParameterObject::WriteParameterFiles does not write the first map! #904

Closed
N-Dekker opened this issue Jun 2, 2023 · 4 comments · Fixed by #967
Closed

ParameterObject::WriteParameterFiles does not write the first map! #904

N-Dekker opened this issue Jun 2, 2023 · 4 comments · Fixed by #967

Comments

@N-Dekker
Copy link
Member

N-Dekker commented Jun 2, 2023

It appears that ParameterObject::WriteParameterFiles skips the first map from the specified parameterMapVector:

ParameterObject::WriteParameterFiles(const ParameterMapVectorType & parameterMapVector,
const ParameterFileNameVectorType & parameterFileNameVector)
{
if (parameterMapVector.size() != parameterFileNameVector.size())
{
itkGenericExceptionMacro(<< "Error writing to disk: The number of parameter maps (" << parameterMapVector.size()
<< ") does not match the number of provided filenames (" << parameterFileNameVector.size()
<< ").");
}
// Add initial transform parameter file names. Do not touch the first one,
// since it may have one already
for (unsigned int i = 1; i < parameterMapVector.size(); ++i)
{
ParameterMapType parameterMap = parameterMapVector[i];
if (parameterMap.find("TransformParameters") != parameterMap.end())
{
parameterMap["InitialTransformParameterFileName"][0] = parameterFileNameVector[i - 1];
}
Self::WriteParameterFile(parameterMap, parameterFileNameVector[i]);
}
}

The bug appears introduced with pull request #50 commit 9ba1683 "ENH: Add convenience methods for writing parameter files", merged on March 20, 2018, when the member function became as follows:

::WriteParameterFile( const ParameterMapVectorType & parameterMapVector, const ParameterFileNameVectorType & parameterFileNameVector )
{
if( parameterMapVector.size() != parameterFileNameVector.size() )
{
itkExceptionMacro(
<< "Error writing to disk: The number of parameter maps ("
<< parameterMapVector.size() << ")"
<< " does not match the number of provided filenames ("
<< parameterFileNameVector.size()
<< ")." );
}
// Add initial transform parameter file names. Do not touch the first one,
// since it may have one already
for( unsigned int i = 1; i < parameterMapVector.size(); ++i )
{
ParameterMapType parameterMap = parameterMapVector[ i ];
if( parameterMap.find( "TransformParameters" ) != parameterMap.end() )
{
parameterMap[ "InitialTransformParameterFileName" ][ 0 ] = parameterFileNameVector[ i - 1 ];
}
this->WriteParameterFile( parameterMap, parameterFileNameVector[ i ] );
}
}

FYI @kaspermarstal

@dzenanz
Copy link
Contributor

dzenanz commented Sep 27, 2023

That probably means that no one has been using that method. And whoever tried to use it, directly or indirectly, gave up on using it because it does not work. The solution is to make it work appropriately 😄

@thewtex
Copy link
Contributor

thewtex commented Sep 27, 2023

I also encountered this issue:

https://github.com/InsightSoftwareConsortium/ITKElastix/blob/29ce6866414070136bb5bc2f70129eafcdde7040/wasm/write-parameter-files.cxx#L74-L76

N-Dekker added a commit that referenced this issue Sep 27, 2023
With this commit `ParameterObject::WriteParameterFiles` writes _each_ maps to a file, including the first one. Aims to address issue #904 "ParameterObject::WriteParameterFiles does not write the first map!"

A GoogleTest unit test is added, testing that each of the files specified by the `parameterFileNameVector` parameter is indeed written to disk.
@N-Dekker
Copy link
Member Author

@dzenanz @thewtex Please check:

N-Dekker added a commit that referenced this issue Sep 28, 2023
With this commit `ParameterObject::WriteParameterFiles` writes _each_ maps to a file, including the first one. Aims to address issue #904 "ParameterObject::WriteParameterFiles does not write the first map!"

A GoogleTest unit test is added, testing that each of the files specified by the `parameterFileNameVector` parameter is indeed written to disk.
N-Dekker added a commit that referenced this issue Sep 28, 2023
With this commit `ParameterObject::WriteParameterFiles` writes _each_ maps to a file, including the first one. Aims to address issue #904 "ParameterObject::WriteParameterFiles does not write the first map!"

A GoogleTest unit test is added, testing that each of the files specified by the `parameterFileNameVector` parameter is indeed written to disk.
@N-Dekker
Copy link
Member Author

I also encountered this issue:
https://github.com/InsightSoftwareConsortium/ITKElastix/blob/29ce6866414070136bb5bc2f70129eafcdde7040/wasm/write-parameter-files.cxx#L74-L76

@thewtex Thanks Matt! Please remember that these two lines should be removed, once ITKElastix has been updated to include elastix PR #967 (commit a57470b):

// The initial is not written by WriteParameterFiles
ITK_WASM_CATCH_EXCEPTION(pipeline, elastix::ParameterObject::WriteParameterFile(parameterMaps[0], parameterFiles[0]));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants