BUG: Fix duplicate output files in Convolution streaming tests#5714
Conversation
|
Thanks @blowekamp Is there any reason for those tests to share the same output directory? Or would it be better if each test would have its own output directory? Something like |
|
Since a year or two ago, each module has its own output directory for tests. Ensuring uniqueness within a module is a lot easier. Most modules do not have enough tests to cause clutter. |
| --compare | ||
| DATA{Baseline/itkConvolutionImageFilterStreamingTestOutput.mha} | ||
| ${ITK_TEST_OUTPUT_DIR}/itkFFTConvolutionImageFilterStreamingTestOutput.mha | ||
| ${ITK_TEST_OUTPUT_DIR}/itkFFTConvolutionImageFilterStreamingTestOutputFrequency.mha |
There was a problem hiding this comment.
Would it be possible to keep the logic of <test-name> + "Output.mha"? When we do it like that, each test will have its own unique output file name. For itkFFTConvolutionImageFilterStreamingTest, it would then still be "itkFFTConvolutionImageFilterStreamingTestOutput.mha" (as it was before).
itkFFTConvolutionImageFilterStreamingValidTest would then have "itkFFTConvolutionImageFilterStreamingValidTestOutput.mha"
Or do you have a specific reason to add the word "Frequency"? (It already has "FFT" as prefix!)
There was a problem hiding this comment.
I just use GH Copilot locally to make these changes. But there appears to be an option "frequency" below and it appears to enable additional logic in Modules/Filtering/Convolution/test/itkConvolutionImageFilterStreamingTest.cxx. This seems like a reasonable additional term for the length filename.
There was a problem hiding this comment.
Maybe Copilot does not understand that each test name is already unique. So it appends an extra word that it can find in the neighborhood. Maybe it's hallucinating 🤷
thewtex
left a comment
There was a problem hiding this comment.
I am surprise we still have this type of bug around.
@blowekamp thanks for addressing it!
| ${ITK_TEST_OUTPUT_DIR}/itkFFTConvolutionImageFilterStreamingTestOutput.mha | ||
| ${ITK_TEST_OUTPUT_DIR}/itkFFTConvolutionImageFilterStreamingValidTestOutputFrequency.mha | ||
| itkConvolutionImageFilterStreamingTest | ||
| frequency |
There was a problem hiding this comment.
@N-Dekker Here is case where the test name is "itkFFTConvolutionImageFilterStreamingValidTest" not the "valid" but here the "frequency" command line argument is used. So there is some pre-existing inconsistencies that could be corrected to make the + "Output.mha" work. But later "valid" is used for the region mode.
Made output filenames unique for FFT convolution streaming tests to prevent file collisions when tests run in parallel. Each test now uses a distinct output filename with 'Frequency' suffix for FFT variants.
b90cf93 to
114b193
Compare
Made output filenames unique for FFT convolution streaming tests to prevent file collisions when tests run in parallel. Each test now uses a distinct output filename with 'Frequency' suffix for FFT variants.
PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.