ismrmrd / siemens_to_ismrmrd

Siemens ISMRMRD converter
Other
39 stars 51 forks source link

Converter trajectoryDescription: Schemas validity error for XA30, but not VE11C #91

Closed mrikasper closed 2 years ago

mrikasper commented 2 years ago

Dear ISMRMRD developers,

We recently switched from a Prisma 3T with software version VE11C to one with XA30. Previously, we were able to use siemens_to_ismrmrd with our own custom spirals, but now we receive the following error for some spirals:

$ siemens_to_ismrmrd -f meas_MID00070_FID06168_diffSpiral_511_b2k_4Avg.dat -z 2

Software version: syngo MR XA30
Protocol name: diffSpiral_511_b2k_4Avg
Dwell time: 5200
Using parameter XSL: IsmrmrdParameterMap_Siemens.xsl   

element trajectoryDescription: Schemas validity error : 
Element '{http://www.ismrm.org/ISMRMRD}trajectoryDescription': This element is not expected. 
Expected is one of ( {http://www.ismrm.org/ISMRMRD}encodedSpace, 
{http://www.ismrm.org/ISMRMRD}reconSpace, 
{http://www.ismrm.org/ISMRMRD}encodingLimits, 
{http://www.ismrm.org/ISMRMRD}parallelImaging, 
{http://www.ismrm.org/ISMRMRD}echoTrainLength ).                                                                 
terminate called after throwing an instance of 'std::runtime_error'                    
what():  Generated XML is not valid according to the ISMRMRD schema

This is our low-resolution spiral; interestingly, the high-resolution spiral (4 interleaves, but only one is used per scan) runs through:

siemens_to_ismrmrd -f meas_MID00076_FID06174_diffSpiral_508_Intl2_b2k_4Avg.dat -Z                                        

Software version: syngo MR XA30 
Protocol name: diffSpiral_508_Intl2_b2k_4Avg
Dwell time: 3300
Using parameter XSL: IsmrmrdParameterMap_Siemens.xsl
Study time: 13:06:24
Last scan reached... 
WARNING: Unexpected number of mystery bytes detected: 32                                 
ParcFileEntries[1].off_ = 40983040                               
ParcFileEntries[1].len_ = 1085101056  
siemens_dat.tellg() = 1126084064                    
Please check the result.                          
WARNING: End of file was not reached during conversion. There are 32 additional bytes at the end of file.

I have attached the diff-file for both xml_raw.xml files, is there anything that could explain the different behavior? diff_xml_raw_76vs70.txt

I have never looked deeply in the XML header and stylesheets before, so my apologies for any naive questions. Here is what I tried:

  1. I installed the latest release of siemens_to_ismrmrd and ismrmrd in a Windows Subsystem Linux, but this does not change the behavior
  2. I changed the .xsl file (interestingly, the default is IsmrmrdParameterMap_Siemens, see above, though the documentation says it should use IsmrmrdParameterMap_Siemens_NX), no luck:
    $ siemens_to_ismrmrd -f meas_MID00070_FID06168_diffSpiral_511_b2k_4Avg.dat -m IsmrmrdParameterMap_Siemens.xml 
    -x IsmrmrdParameterMap_Siemens_NX.xsl -z 2  
  3. I looked for obvious differences in the outputs of the xml_raw.xml files, but apart from system/sequence specifics (other diffusion gradient b-value, directions, TR), I didn't not see anything suspicious between the VE11 and XA variants. The two spirals on the same system only differ in the used gradient waveform (read from a text file), and the diff is attached. I could provide the full files, if this is helpful.
    $ siemens_to_ismrmrd -f meas_MID00070_FID06168_diffSpiral_511_b2k_4Avg.dat -z 2 -X
  4. Could <ucTrajectory>4</ucTrajectory> be an issue? I believe this is SPIRAL, but we are not using Brian Hargreaves's parameterization for the gradient waveform, so also didn't populate such parameters. In general, is it better to use "other" in such cases?

Thank you for your help and

All the best, Lars

hansenms commented 2 years ago

If you run the converter with -X and look at the generated ISMRMRD header, what does it look like in the two cases.

I think it is trying (as you say based on trajectory being spiral) to do "something" it shouldn't really be doing and it might not turn out right. You may have to make your own stylesheet to just skip that part or make it do what you would like it to. If you can provide links to the *.dat files, I can also take a look.

mrikasper commented 2 years ago

Dear Michael,

Thank you so much for the swift response! The .mrd file is not written out for the case were siemens_to_ismrmrd fails, also not with the -X option.

I only get the xml_raw.xml files - and I have to specify -z 2, because otherwise it converts the calibration scans:

siemens_to_ismrmrd -f meas_MID00070_FID06168_diffSpiral_511_b2k_4Avg.dat -X -z 2     
siemens_to_ismrmrd -f meas_MID00076_FID06174_diffSpiral_508_Intl2_b2k_4Avg.dat -X -z 2                                                                         

I have attached both xml_raw.xml files here, the diff is above. The only other file that is generated is the config_buffer.xprot, I have created a diff here. If nothing jumps out to you there, I can definitely share the .dat files, just give me a moment.

What is puzzling me, is that one spiral works, but the other one doesn't.

Also, regarding the creation of one's own stylesheet, what is a good starting point to learn about this? How do I understand what the error above means in terms of adapting the stylesheet?

Thank you for your help again!

All the best, Lars xml_raw_76_rename2xml.txt xml_raw_70_rename2xml.txt diff_config_buffer_76vs70.txt

hansenms commented 2 years ago

@mrikasper it is a bit hard to dig in without having the *.dat files. But here is what I suspect:

From what I can see from your error message you are encountering a trajectoryDescription section of the ISMRMRD XML header before you see another expected section (encodedSpace).

A side comment: There is actually a bit of a slight bug with the stylesheets we have in the converter in that we have the trajectory and trajectory description section before the encodedSpace section, but the converter "fixes" this by deserializing the output of the stylesheet rendering and then later serializing it, so things end up in the right order. This "magic" happens here (https://github.com/ismrmrd/siemens_to_ismrmrd/blob/master/main.cpp#L832).

That said, some of this end up not working out in your case, but you are flying blind, because you never the the processed.xml file and so you don't really know what is happening. Here is what I would do to debug this:

  1. Move the section where you write out the processed.xml to right before you validate it. So this line: https://github.com/ismrmrd/siemens_to_ismrmrd/blob/master/main.cpp#L910 should up above the code right about where you validate the xml.
  2. Let the conversion run (and fail again).
  3. Take the processed.xml and validate against the XSD schema file for ISMRMRD:

      xmllint --schema <PATH TO WHERE ISMRMRD IS INSTALLED>/share/ismrmrd/schema/ismrmrd.xsd processed.xml
  4. That should give you the same error as you are seeing?
  5. Now start working your way backwards and figure out what is missing in your header, etc.

Again, I cannot really walk this path since I don't have the files, but lemme know if it makes sense and what you find.

hansenms commented 2 years ago

Actually, I have found your problem. In the diff, I see this:

1527,1533c1527,1533
<       <ParamLong."alRegridRampupTime">{ 0 0 }
<       <ParamLong."alRegridRampdownTime">{ 0 0 }
<       <ParamLong."alRegridFlattopTime">{ 0 0 }
<       <ParamLong."alRegridDelaySamplesTime">{ 0 0 }
<       <ParamDouble."aflRegridADCDuration">{ 0 0 }
<       <ParamLong."alRegridDestSamples">{ 0 0 }
<       <ParamLong."alRegridMode">{ 1 1 }
---
>       <ParamLong."alRegridRampupTime">{ 80 0 }
>       <ParamLong."alRegridRampdownTime">{ 80 0 }
>       <ParamLong."alRegridFlattopTime">{ 1180 0 }
>       <ParamLong."alRegridDelaySamplesTime">{ 4 0 }
>       <ParamDouble."aflRegridADCDuration">{ 1331.199951 0 }
>       <ParamLong."alRegridDestSamples">{ 256 0 }
>       <ParamLong."alRegridMode">{ 2 1 }

If you look at the style sheet used, this will cause the stylesheet to insert an EPI section in the XML header in addition to the Spiral section already being inserted. Look at these two places:

https://github.com/ismrmrd/siemens_to_ismrmrd/blob/master/parameter_maps/IsmrmrdParameterMap_Siemens.xsl#L239 https://github.com/ismrmrd/siemens_to_ismrmrd/blob/master/parameter_maps/IsmrmrdParameterMap_Siemens.xsl#L287

In your case, you probably want neither of those, so I would extract the stylesheet in question:

siemens_to_ismrmrd --extract IsmrmrdParameterMap_Siemens.xsl

(or just download it from the repo).

I would then completely remove those two sections that are adding trajectory descriptions. Save the new stylesheet, e.g. as IsmrmrdParameterMap_Kasper.xsl (for example). And now do the conversion with this stylesheet:

siemens_to_ismrmrd -f meas_MID00070_FID06168_diffSpiral_511_b2k_4Avg.dat -X -o test.h5 -x IsmrmrdParameterMap_Kasper.xsl -z 2

Hope this helps?

rajramasawmy commented 2 years ago

I have a vague memory that the converter will sometimes also try to design a Hargreaves variable density spiral - and that has occasionally thrown some errors at me in custom configuration (I may have been doing other things wrong!). Just a general question here - should we still have that feature in the main converter?

hansenms commented 2 years ago

@rajramasawmy we should probably remove that or at least make it optional. But in this case, it is doubly bad because tries not only to design Hargreaves spirals but also an EPI trajectory. Neither of them is what @mrikasper wants in this case, but having both is even worse. But yes, we should probably remove this or make it optional. There are some downstream effects, but we can discuss how we mitigate.

mrikasper commented 2 years ago

Dear Michael @hansenms,

This is brilliant, thank you so much for digging into that so thoroughly and quickly! I loop in @alexjaffray here, who is working with me on our spiral recon pipeline.

I tried your suggestion, and just removing the bit on the alRegridRampupTime in the .xsl file led to a successful conversion (see below and attached). I now understand the purpose of the style sheets much better as well!

$ siemens_to_ismrmrd -f meas_MID00070_FID06168_diffSpiral_511_b2k_4Avg.dat -X -z 2 -x IsmrmrdParameterMap_Siemens_NX_ReadinSpiral.xsl 

Software version: syngo MR XA30
Protocol name: diffSpiral_511_b2k_4Avg
Dwell time: 5200
Using parameter XSL: IsmrmrdParameterMap_Siemens_NX_ReadinSpiral.xsl
Study time: 13:01:54
Last scan reached...
WARNING: Unexpected number of mystery bytes detected: 32
ParcFileEntries[1].off_ = 40983040
ParcFileEntries[1].len_ = 1054123872
siemens_dat.tellg() = 1095106880
Please check the result.
WARNING: End of file was not reached during conversion. There are 192 additional bytes at the end of file.    

From a more general point of view, @hansenms and @rajramasawmy, I had 3 thoughts:

  1. I was wondering whether these side effects in the style sheets are a good thing. They probably enrich conversion a lot for standard sequences, but then they create these rather fuzzy errors when prototyping. Granted, we didn't clean up our parameter sheet when repurposing our EPI sequence for spiral acquisition, but this is probably not uncommon while you are developing. Would there be any harm to make the side effect conditional on the trajectory type (e.g., for EPI or Cartesian only?). The way I understand it, even if our sequence had been labeled via ucTrajectory as "custom", this error would have occurred, wouldn't it?

  2. Beyond this, maybe labeling Brian Hargreaves's spiral parameterization as SPIRAL is indeed too restrictive. One could have SPIRAL for the general info and maybe HSPIRAL (or HVDSSPIRAL) for the specific parameterization (is there any public registry for extending the existing ucTrajectory type?)

  3. Finally, even if there are conflicting header parameters in a prototype sequence, could the converter fail "gracefully", i.e., create the .mrd file, but produce some warnings about unresolved conflicts and leave the ambiguous fields UNDEFINED or similar? This could either be the default or a command line flag that uses a minimal/debug style sheet (e.g., IsmrmrdParameterMap_Siemens_DEBUG.xsl) avoiding any side effects?

Once again, thank you for your fast and thorough help, I am really a big fan of ISMRMRD as a vendor agnostic format going into any reconstruction pipeline.

All the best, Lars

IsmrmrdParameterMap_Siemens_NX_ReadinSpiral_rename2xsl.txt

hansenms commented 2 years ago

Hi @mrikasper,

Glad you got it working. Some comments on your general comments:

  1. There would be no harm in adding addition conditions on various sections of the stylesheets. As with most of these things they work pretty well for the mainstream cases but in prototyping we see more corner cases. I would very much encourage you and your team to make contributions in the form of pull requests to get better checks and conditions in the style sheet. This is a community project where the cases that work the best are the cases that the contributors are interested in. But it is all open source and we would love to get more contributions. They are best made by the people experiencing the corner cases, etc. So no, there would be no harm in making this better, so feel free to put up a PR and we will take a look.
  2. I agree, the Brian Hargreaves spiral should not be the "default" case whenever spirals are encountered, however, following on my comments above, the stuff that works well is the stuff that the developers have cared about and used themselves. So I would love to see a revamp of how we handle spirals and other trajectories, but it should be driven by people with the use cases. We could remove the spiral stuff from the basic stylesheets and have a one in there for those types of spirals that people could use. I am definitely open to reworking this, if somebody would like to take that on.
  3. On this one, it would be a pretty hard no from me. I don't think the converter should be able to generate an mrd file with an invalid header in it. Knowing the community, we would start seeing a lot of these files around because getting it right is sometimes hard and the easy path is just to have a header with garbage in sections you don't care about. The gremlins would get in pretty quickly, so there is nothing "graceful" imo about a header that is incorrect. That said we could (for instance) shift the debug stuff around a bit (as discussed above) so that we at least dump the invalid header to disk for debugging purposes. Again, happy to take a PR on that.

I would really encourage you and your team to contribute to the project, it sounds like you have some great use cases and could help out a lot. I also understand that this is no ones day job, so I will not push too hard. But lemme know if you want to have a chat about improving the trajectory support and we can come up with a workable plan.

I will close this issue for now. And if you want to improve some of the style sheets, we will be standing by to review the PRs.

mrikasper commented 2 years ago

Dear Michael @hansenms,

Thank you for your detailed thoughts on this. I agree with points 1 and 2 and will do my best to encourage the people here to contribute to ISMRMRD with improvements that reflect our use case. You have given good pointers on how to do so in this thread, so thank you a lot for this!

Regarding point 3, I understand the worry of having a non-standardized version of a raw data standard completely :-)

I think my line of thinking came from the workflow I have experienced now both on Philips and Siemens systems:

So ultimately, I do believe there is a need for having such intermediate outputs, but maybe by specifying a few parameters right (like ucTrajectory), the .mrd files are consistent enough to be used in that sense. But you are right, it might be good to present our use case in more detail and think about it together. Is the weekly gadgetron meeting the right place? Otherwise, @alexjaffray is also presenting our pipeline at ISMRM.

All the best, Lars

hansenms commented 2 years ago

I think it may be a question of defining the roles of different tools. I have no doubt there are applications where more tweaking and code is needed to generate valid datasets and scripting environments may in fact be better in those cases. This is not the only converter out there and there may be needs for other, but I think this one should always produce either a valid dataset or an error. You can always give it a stylesheet that produces a dummy but valid header and then have pass in some other tool that may augment the data in some way. But a failure to produce a valid header should be an error here.

In terms of where to discuss further, we have a meeting on Mondays (11am Pacific) for people working on the data format. We often have a fair bit to discuss there, so I would suggest setting up some separate time. To get that rolling, email me (mihansen AT microsoft.com) and we can work through there.

In general, I would suggest that you write up something ahead this. A simple markdown file (e.g. on hackmd.io) containing:

  1. Justification: Explain WHY we are building this feature. WHO is this for?
  2. Scenarios: List the scenarios this will enable. These should map to acceptance criteria and associated tests.
  3. Design: Describe the design with enough detail that code reviewers and stakeholders are not surprised.
  4. Test Strategy: Describe how we test this.
  5. Maintenance: What maintenance issues might there be with this feature and WHO will maintain it?
  6. Other: Any other issues

Hope this helps, Michael

mrikasper commented 2 years ago

Excellent, thank you, Michael!

It is on my todo-list now, but it might be some time (early April) to think this through carefully and create the structured document, as you suggested. Maybe it turns out there is not so much to do (other than a few .xsl and the swap of processed.xml with its validation), once I have experimented a bit more with our current workflow.

All the best, Lars