InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.37k stars 660 forks source link

Parse strings as multi-element data structures in tests #514

Open jhlegarreta opened 5 years ago

jhlegarreta commented 5 years ago

Description

Some tests require a number of arguments that are part of the same data structures (e.g. the image size, or resolution). Having a utility function in a single place to parse the input parameter strings as a multi-element data structure (e.g. a vector) would be desirable.

Expected behavior

To be able to parse a string as a vector or whatever multi-element data structure.

Actual behavior

Developer's are forced to parse the string on each test that requires such an input.

dzenanz commented 2 years ago

Inspiration for this could be found somewhere in https://github.com/Slicer/SlicerExecutionModel.

dzenanz commented 2 years ago

@jhlegarreta adding a few example tests where this could be applied would make it a "good first issue".

jhlegarreta commented 2 years ago

@jhlegarreta adding a few example tests where this could be applied would make it a "good first issue".

I should have done this when I opened the issue, I am really sorry. I had a hard time finding examples of these cases, either - because I am having a hard time remembering what I meant 🤦‍♂️,

But I think these cases illustrate what I meant (when git blaming the files, the commits that added the parser methods turn out to be mine, so probably this issue partially arose from writing them 🙃): https://github.com/InsightSoftwareConsortium/ITK/blob/23da3dfa79152648f3d38050ee147df47f41ce0a/Modules/Core/ImageFunction/test/itkBSplineDecompositionImageFilterTest.cxx#L37 https://github.com/InsightSoftwareConsortium/ITK/blob/23da3dfa79152648f3d38050ee147df47f41ce0a/Modules/Filtering/Denoising/test/itkPatchBasedDenoisingImageFilterTest.cxx#L37

This one might also be relevant: https://github.com/InsightSoftwareConsortium/ITK/blob/23da3dfa79152648f3d38050ee147df47f41ce0a/Modules/Video/IO/test/itkFileListVideoIOTest.cxx#L305 This one is an example file, not a test (but the same principle may apply): https://github.com/InsightSoftwareConsortium/ITK/blob/23da3dfa79152648f3d38050ee147df47f41ce0a/Examples/Filtering/DigitallyReconstructedRadiograph1.cxx#L126

So (I think that) the idea was/is that instead of specifying e.g. the image size test parameter as imageSizeX imageSizeY imageSizeZ we could specify it as imageSize (in the test usage), specifying the argument in the CMakeLists.txt as e.g. "10 20 30" (instead of 10 20 30), and have a method to parse the string and fill the image size array. Now this assumes that the e.g. image dimension is either inferred from the test parameters or is added as an explicit parameter (the latter may make things easier, but gives room for incurring into more inconsistencies I think). This makes the test less explicit, since we do not know the image dimensionality by just reading the test (we need to go to the CMakeLists.txt file), but gives way to potentially increase the number of tests done without duplicating the code.

As an example, the following is a test where the spacing is required as 3 separate parameters: https://github.com/InsightSoftwareConsortium/ITK/blob/23da3dfa79152648f3d38050ee147df47f41ce0a/Modules/IO/GDCM/test/itkGDCMSeriesStreamReadImageWriteTest.cxx#L54

In the case of other lists that do not influence the dimensionality of the template parameters, like in https://github.com/InsightSoftwareConsortium/ITK/blob/23da3dfa79152648f3d38050ee147df47f41ce0a/Modules/Core/ImageFunction/test/itkBSplineDecompositionImageFilterTest.cxx#L37, the need for the parser is clear I think. These would also apply to e.g. some rigid/affine transformation matrices that one would be willing to test (or these could be stored in e.g. txt files, but the latter should also get parsed, and such parsers be common/re-used).