Tckconvert write binary vtk #2489
Conversation
New default is to write binary VTK files, the testing needed to be changed to reflect this.
…ck with original New tests are: 1. convert tck -> vtk -> txt and compare to tck -> txt 2. convert tck -> vtk -> txt and compare to tck -> vtk -> tck -> txt
To maintain consistency with current behaviour. This can (should?) be changed to default to binary in the next minor release (3.1.0).
to guarantee automatic deallocation in all cases.
jdtournier
force-pushed
the
tckconvert_write_binary_vtk
branch
from
e5d74a4 to
de51453
Compare
|
@jdtournier LGTM. Preserving backwards compatibility by using |
testing/binaries/tests/tckconvert
Outdated
| tckconvert tracks.tck tmp.vtk -binary -force -quiet && tckconvert tmp.vtk tmp-vtk-streamlines-[].txt -force && cat tmp-vtk-streamlines-*.txt > tmp-vtk-all.txt && tckconvert tracks.tck tmp-mrtrix-streamlines-[].txt -force -quiet && cat tmp-mrtrix-streamlines-*.txt > tmp-mrtrix-all.txt && testing_diff_matrix tmp-vtk-all.txt tmp-mrtrix-all.txt | ||
|
|
||
| # convert tck -> vtk -> txt and compare to tck -> vtk -> tck -> txt | ||
| tckconvert tracks.tck tmp.vtk -binary -force -quiet && tckconvert tmp.vtk tmp-roundtrip.tck -force && tckconvert tmp-roundtrip.tck tmp-roundtrip-streamlines-[].txt -force && cat tmp-roundtrip-streamlines-*.txt > tmp-roundtrip-all.txt && tckconvert tracks.tck tmp-mrtrix-streamlines-[].txt -force -quiet && cat tmp-mrtrix-streamlines-*.txt > tmp-mrtrix-all.txt && testing_diff_matrix tmp-roundtrip-all.txt tmp-mrtrix-all.txt |
There was a problem hiding this comment.
One lesson I've learned the hard way elsewhere is that if your own software is non-compliant, but the reader and writer functionalities have matching non-compliance, then round-trip tests do you no good.
If we could add an exemplar binary file into the test_data repository that demonstrably works in some other software, then we could ensure that that at least gets read correctly. Writing back to binary and then doing a binary diff might work for testing the write.
I don't think it's super critical here given that we presume you've implemented the writer for a purpose and it's being successfully read by something else, and also that the code is relatively independent of everything else. But as with another comment, for #411 we'd need to be more exhaustive.
There was a problem hiding this comment.
Indeed. I'll add the outputs produced by the conversion to the test data repo and test against that. It'll catch regressions, and if @blezek can double-check that these work correctly in other packages, we should be good.
There was a problem hiding this comment.
OK, done. @blezek, any chance you could sanity check the binary VTK test data before we merge?
There was a problem hiding this comment.
I don't have anything with which to visualise polylines.
To avoid segfault in case vector is empty.
@jdtournier @Lestropie sorry, this PR got away from me. I verified the binary VTK file loads correctly in ParaView, so we are good to go! |
replaces #2488, this time merging to
masterwith VTK output still defaulting to ASCII.@blezek, could do with a second pair of eyes to double-check none of my changes break anything...