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

ENH: writing data file can be skipped in some circumstances #753

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dzenanz
Copy link
Contributor

@dzenanz dzenanz commented Dec 24, 2020

This is a PlusLib companion of IGSIO/IGSIO#21.

@nathanbmnt
Copy link

nathanbmnt commented Jan 18, 2021

I tried this patch with MHD and many of the headers are being overwritten with default values. I used the command

--source-seq-file="C:\Users\natha\Documents\SonoEQ\AA_3D Scan_2019_09_12-15_53_25\UnfusedVolume.mhd"
--output-seq-file="C:\Users\natha\Documents\SonoEQ\AA_3D Scan_2019_09_12-15_53_25\UnfusedVolume.mhd"
--operation=UPDATE_FIELD_VALUE
--field-name=sonoEQ.Hardware.version
--updated-field-value=myValue
--maintain-custom-headers
"sonoEQ.Hardware.version"

On the right is the mhd before the command, on the left is after.
mhdscreenshot

Also, if you use this command with .mha's, the same thing happens to the headers and the image data is deleted.

@adamrankin
Copy link
Member

@dzenanz could you confirm this?

@dzenanz
Copy link
Contributor Author

dzenanz commented Jan 19, 2021

I will look into this.

@dzenanz
Copy link
Contributor Author

dzenanz commented Jan 19, 2021

With a file I have lying around I could observe ElementSpacing changing from 0.1 -0.0385753 0.1 to 1 1 1. While I debug that, @nathanbmnt can you share AA_3D Scan_2019_09_12-15_53_25\UnfusedVolume.mhd (just the header)?

@dzenanz
Copy link
Contributor Author

dzenanz commented Jan 19, 2021

ElementSpacing changes from 0.1 -0.0385753 0.1 to 1 1 1 even before this patch. I guess that vtkIGSIOTrackedFrameList does not record, and therefore does not preserve ElementSpacing. That tag is only handled per-frame. I could add a commit which tries to preserve that if frame ElementSpacing is set.

@dzenanz
Copy link
Contributor Author

dzenanz commented Jan 19, 2021

Force-push just rebases the commit on top of current master.

@dzenanz
Copy link
Contributor Author

dzenanz commented Jan 19, 2021

I pushed a new commit for ElementSpacing change in IGSIO/IGSIO#23.

@nathanbmnt
Copy link

@dzenanz here is the file

https://drive.google.com/file/d/1Nizde_rPzhwxdMr7ncUN5aiEdxolTAZi/view?usp=sharing

Notice that in the above screenshot CompressedData and dimSize are also changed

@dzenanz
Copy link
Contributor Author

dzenanz commented Jan 20, 2021

There are some changes for me (using updated branches after rebase), but dimSize is unchanged:
Screenshot 2021-01-20 13 33 57
Whether to write the output file compressed is controlled by --use-compression command line parameter to EditSequenceFile. If it is provided, the header has:
CompressedData = True and CompressedDataSize = 0 (many extra spaces for the size to be calculated). I agree that this case is broken, but I could not figure out whether the input file was compressed from read function:

if (vtkPlusSequenceIO::Read(inputFileNames[i], timestampFrameList) != PLUS_SUCCESS)

@dzenanz
Copy link
Contributor Author

dzenanz commented Jan 20, 2021

The force-push changes the logic so in-place change of the header is not attempted if compression is requested.

@jamesobutler
Copy link
Contributor

The volume in the example with an element spacing that wasn't ElementSpacing = 1 1 1 was a volume output from VolumeReconstructor.exe which is a static volume and not a Sequence volume. Therefore it was probably correct behavior for EditSequenceFile.exe to save it as though it was saving a sequence as PLUS usually does when writing volumes with VirtualCapture. Otherwise trying to maintain element spacing field from the input would probably make this tool more of a general Volume Header edit tool whether it is a sequence or a static volume.

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 this pull request may close these issues.

4 participants