MGH / MGZ: alternative implementation using normal file-based IO #970
Conversation
Creating .mgh / .mgz images with little-endian encoding causes faults in other softwares; therefore enforce big-endianness regardless of header datatype.
Consistently read flip angle in radians and convert to degrees for header storage, and convert from degrees to radians when writing from header into MGHO.
- remove detection of byte order, now hard-coded to Big-endian as per documentation; - use sized types throughout in MGH header structures to ensure correct byte swapping, etc.; - convert tag IDs to text and vice-versa for known tags, and substitute with raw number if unknown; - general cleanup to maximise code reuse between .mgh and .mgz formats.
Note that now the contents of struct mgh_tag will appear as big-endian in all ciircumstances, and therefore conversion is necessary whenever reading or writing; this is to be consistent with the other mgh header structures.
jdtournier
force-pushed
the
mgh_mgz_write_fix_take2
branch
from
d340fa5 to
29c93a9
Compare
|
OK, got FreeSurfer installed, and tested this with One final change I had planned was to substitute suitable alternative datatypes when those requested aren't available. Most other formats handle this behind the scenes to avoid causing extra confusion. Ideally, I'd like the backend to figure out when the datatype has been explicitly requested by the user and error out when that request can't be fulfilled, while substitute a suitable alternative otherwise, but that's another story... |
|
OK, had a go at dealing with the datatype substitution, and also added a warning in case datatype has been substituted (for any format, not just MGH/MGZ). Hopefully that'll be it for this pull request... |
- Clearer mapping from requested header datatype to MGH format datatype on write. Removed errant VAR() call.
Lestropie
left a comment
Lestropie
left a comment
There was a problem hiding this comment.
Survived my testing.
My remaining concern though is that the reason this version creates files that can be read by FreeSurfer is that it preserves all the null filling. This is fine in terms of not crashing FreeSurfer on a direct convert, but it means that e.g. gedit damn near hangs if you try to open a .mih file containing those comments, and e.g. if those comments are edited in any way (or if a user were to add comments prepended with "MGH_TAG_") there's every possibility that FreeSurfer could then crash again. Not the end of the world, but if I had more time I'd have preferred to convert to & from MGH/MGZ while keeping those comments human-readable.
Yes, it does. But I'm not sure how FreeSurfer interprets some of these fields. The And I agree that there's always scope for someone to mess with the tags by adding their own |
|
Did some prodding...
|
|
OK, given the above, my gut feeling is it's safest just to keep them as-is - at least for now. We might be able to revisit later. On the other hand, there's a strong case to be made for getting this right from the outset. If we allow conversion from MGH to On a different note, any opinions on my previous suggestion below?
|
If we want to get it right: If we want to merge in the next 7 days and include this I don't like my own chances, I've barely started my Thursday talk slides and haven't done a run through of Monday's talk. Question then is whether or not this is more critical than doing the merge for ISMRM (bearing in mind the PR started in September...)
Not sure that really solves any fundamental issues. Could still be modified via |
😵 14,000 lines of code! That's a monster...
I'm starting to feel that way too... Thinking about it, I think we really want to avoid introducing changes now that will be changed again later, and screw up the handling of data converted with the different versions. Best to hold on with these changes, deal with the big merge, finalise all of the modifications we want to do to the MGH handling (including updating #962) , and merge those changes at a later time - as you say, they're not critical for the big merge...
It's not supposed to solve any of these fundamental issues, it's more about where we think that information ought to live. We currently store them as comments, but they're not really comments as such... Likewise, the TE/TR/TI/flip angle fields might also be better stored as dedicated entries in their own right, but in this case we have to agree on the correct header key, since it would essentially become part of the But in any case, since we'll probably be delaying these changes till after the big merge, we can discuss these issues further at that point. |
|
Worth checking this file also. |
My initial reaction was to leave them as comments, since I don't think we want the nuances of other people's formats 'intruding' into our own. But on second thought, they are key-value pairs after all, so making them standard entries makes more sense in that respect. In terms of naming, I'd prefix all of them (including TE, TR, TI, flip angle, FOV) with |
Fixes for compiling against 3.0_RC1 branch. Moved contents of MGH format "other" field from header comment entries to header key/value pairs. Add contents of MGH_TAG_CMDLINE to header key/value entry "command_history" as in #962, and vice-versa.
Write tag data in the same order as what appears to be performed in the FreeSurfer code: transform matrix, then general tags, then cmdline history.
Mimic FreeSurfer's own code in order to properly import additional header data that are stored in the 'other' section, and convert all data into text in order to be readable as header key-value entries. Convert back to binary when writing to MGH / MGZ format.
|
@jdtournier: I probably spent too much time on it, but I've mimicked the FreeSurfer import / export code for each tag, and converted the content of each to/from text form so that they're all readable in the header key/value entries. |
|
(don't mind the commits, they just bring the copyright message for the new files up to date) |
|
I'm content that this is doing as it should. For all three problematic images I've been provided with, I can convert from MGZ to |
As long as all current evidence shows this works, I'd go ahead and merge asap. If crashes are fixed, that's quite important. Testing can continue independently of course. |
Echoes changes introduced in #970.
This is an alternative solution to #956, using standard
read()/write()calls to avoids a few oddities with using memory-mapping to read the MGH data. This allows a lot more re-use of the code between the compressed and non-compressed cases, and seems to work well on my system. The code also seems more readable (to me, at least...)Yet to be tested thoroughly against FreeSurfer, but as far as I can tell, the output is bit-wise identical, apart from a rounding difference in the
TEentry, and a zeroFOVentry (which is stated as unused in the docs). So if things fall over with FreeSurfer, that zeroFOVentry is likely to be the problem...We might also need to add a bit more error detection, particularly to avoid cases where there is no data at the end of the file. I'm not sure what would happen in this case currently...