Converter trajectoryDescription: Schemas validity error for XA30, but not VE11C about siemens_to_ismrmrd HOT 11 CLOSED

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.

from siemens_to_ismrmrd.

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

from siemens_to_ismrmrd.

@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.

from siemens_to_ismrmrd.

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?

from siemens_to_ismrmrd.

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?

from siemens_to_ismrmrd.

@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.

from siemens_to_ismrmrd.

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

from siemens_to_ismrmrd.

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.

from siemens_to_ismrmrd.

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:

• It is harder to program data headers etc. correctly on the vendor-specific pulse programming side than to fix them once they are in an easy-to-use or interpreted high-level language (Matlab, Python, Julia).
• But in order to do that, we need an entry point to get data and header in these environments, and <vendor>_to_ismrmrd does just that nicely.
• Fundamentally, there are also cases where the "correct" header cannot be known at scanner runtime. For example, if trajectory and coil data are from different vendors (e.g., using a magnetic field camera @SkopeMagneticResonanceTechnologies), the raw data file coming from only one of them cannot really reflect the true state of the acquisition, and some form of merge is mandatory. This is basically what we are doing now with our read-in gradient files (later incorporating some GIRF prediction as well).
• Of course one can do this with ideas such as <vendor1>_and_<vendor2>_to ismrmrd datafile.vendor1 datafile.vendor2, but since these data formats change so rapidly, I think an interpreted language is again better suited for this effort than having to compile this for different architectures and all combinations.

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

from siemens_to_ismrmrd.

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

from siemens_to_ismrmrd.

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

from siemens_to_ismrmrd.