Various DICOM updates

[jdtournier]

Simple fix for issue reported on the forum, which lead to incorrect sorting of dMRI data for some Philips datasets.

[Lestropie]

Looks limited to Philips and DWI, and the field name (already present in our dictionary) certainly checks out.

Having multiple DICOM fields write to the same member variable however makes me nervous. In similar circumstances for other types of data (e.g. slice timing comes to mind), what I've done is to have different DICOM fields write to different member variables, and then only at the point of constructing the series, interrogate which fields have been filled comprehensively by which sources, and set the precedence of which shall be utilised if more than one source is available. Would this be a safer approach here?

[jdtournier]

I had similar reservations, but convinced myself that this particular variable (sequence) is currently only used in one other contexts, which is specific to older Siemens data - where the only way to sort the series correctly was by interpreting the sequence name string (0x0018, 0x0024 - SequenceName), which had a format like b1000#2 where the last number indicates which volume the slice belongs to.

But thinking about it further, you might be right that in some contexts where the SequenceName entry is set to a parsable value, this might cause issues. I'll add another variable for this to avoid this kind of issue.

[jdtournier]

We've also got much bigger problems in that the DICOM test suite fails currently, on master and this pull request. I have a feeling this might date back to #2015 - which was quite a while ago... Maybe there's some other more recent change that would explain this, but nothing stands out right now...

[jdtournier]

OK, on closer inspection, I think the issue that screwed up the DICOM testing for the DW encoding info was #2231. Regardless, I'll need to look into this closely to figure out whether it's OK simply to update the test data.

Also need to consider the artificial isotropic-weighted images that Philips likes to add at the end of their series (#2172). So far, I really can't see how to reliably do this - but I'll discuss that on the relevant issue.

[jdtournier]

You'll note that I've added a solution to #2172 in that last commit (5df4c98) - see this comment for a full description of the thought process...

[jdtournier]

OK, seems there was another bug introduced in #2015: when there is a b=0 volume with zero direction vector, the detection of whether b-value scaling should be applied goes wrong, because the abs log of the vector norm is infinite - hence always triggering b-value rescaling... Fixed with e105aec.

[jdtournier]

Turns out that last change breaks a lot of the testing, since the DWI dataset we use for testing was affected... Just to clarify:

current master:

$ mrinfo testing/binaries/data/dwi.mif -dwgrad
                   0                    0                    0                    0
                   0                    0                    0                    0
 -0.0509540919293102    0.061755090218525   -0.996789842117064     2950.00093450932
  0.0119069972166564    0.955046776751157    0.296215930757461       3000.001402542
   -0.52511512453343    0.839985199206294    0.136671032412154       2999.998577073
  -0.785445219860643    -0.61110017105824  -0.0981447274725243     2999.99832048927
  0.0608619958084041   -0.456700968546777    0.887535938874958       3000.000413223
   0.398325005929864    0.667699009940035    0.628900009362435       2999.999910678
  -0.680603594422654     0.68964458903505   -0.247323852617658       3000.003575451
   0.237398967996455    0.969994869235849   0.0524564929283866     3000.00080885475
...

this branch:

$ mrinfo testing/binaries/data/dwi.mif -dwgrad
                   0                    0                    0                    0
                   0                    0                    0                    0
 -0.0509540919293102    0.061755090218525   -0.996789842117064                 2950
  0.0119069972166564    0.955046776751157    0.296215930757461                 3000
   -0.52511512453343    0.839985199206294    0.136671032412154                 3000
  -0.785445219860643    -0.61110017105824  -0.0981447274725243                 3000
  0.0608619958084041   -0.456700968546777    0.887535938874958                 3000
   0.398325005929864    0.667699009940035    0.628900009362435                 3000
  -0.680603594422654     0.68964458903505   -0.247323852617658                 3000
   0.237398967996455    0.969994869235849   0.0524564929283866                 3000
...

Guess we're going to have to update some of the test data as well...

[jdtournier]

Right, looks like this is pretty much good to go. Decision required as to whether to go with this PR (see description in 4223ae6) or #2450. I personally favour #2450, I expect everyone else will too... Good news is both approaches check out, which gives us confidence that the updated test data on #2450 is correct. Can I get someone to approve #2450 (or this one if that's the preferred option)?

Quick recap of the changes here:

• fix sorting issue on some Philips data (92e4885, f48553c)
• strip out trace-weighted scan from Philips data (with option to preserve) (5df4c98)
• fix bug in detection of whether b-value scaling should be applied (e105aec)

I realise these should probably all be merged as independent PRs, but this should be OK as a single PR if I edit the title to e.g. 'various DICOM updates', right...? 🙏

[Lestropie]

@jdtournier Close?

[jdtournier]

👍