FIX: Restore SyN-SDC

[effigies]

Changes proposed in this pull request

Documentation that should be reviewed

[mgxd]

Had a quick look, but looks like ds005 doesn't undergo any fieldmap processing, despite the inclusion of the --use-syn-sdc flag. Not sure why though..

[effigies]

Yeah, I'm trying to debug the workflow creation to figure out what's failing.

[effigies]

So I think the issue is that we now require PhaseEncodingDirection to run. Adding it to ds000005, I start accessing these paths.

[effigies]

@oesteban It seems that we're no longer able to run SyN without the following defined for a BOLD file:

• PhaseEncodingDirection
• One of:
  • TotalReadoutTime
  • EffectiveEchoSpacing
  • EchoSpacing and ParallelReductionFactorInPlane
  • WaterFatShift and EPIFactor

Presumably we should be able to have some kind of fallback, since we were able to do it before. What would be the best way to do this in the new SDCFlows?

[effigies]

Okay, patched in the metadata. It's a little hacky, but I couldn't readily see a cleaner way of doing it.

[effigies]

Well, it runs, but it's a substantial regression:

[oesteban]

Sorry for the slow response.

@oesteban It seems that we're no longer able to run SyN without the following defined for a BOLD file:

* `PhaseEncodingDirection`

* One of:
  
  * `TotalReadoutTime`
  * `EffectiveEchoSpacing`
  * `EchoSpacing` and `ParallelReductionFactorInPlane`
  * `WaterFatShift` and `EPIFactor`

Presumably we should be able to have some kind of fallback, since we were able to do it before. What would be the best way to do this in the new SDCFlows?

This is intended. You could convince me to allow setting TotalReadoutTime to 1.0 if it is not found in the metadata, but we should stop inferring AP/PA when phase encoding direction is not present. SDCFlows (or any other tool) should not by any means attempt correction with the assumption that distortion only happens along one axis if that axis is unknown.

The second part of the problem (the readout time) could be worked-around - but big fat warnings should be set in place. Here's the problem: now SDCFlows calculates a fieldmap and the actual value of the readout time is necessary to write out the fieldmap in Hz. Since it is a scaling factor, if all runs of the dataset are acquired with exactly the same readout time, it can safely be set to 1.0 in estimation of the fieldmap and reconstruction of the distortion. However, these fieldmaps should not be used for anything else (e.g., averaging fieldmaps to calculate a new "standardized" average across studies) because the units (or more precisely, the scale) are unknown.

a substantial regression:

This is expected, there are some substantial changes made to the workflow that have made it very different from the original implementation. The most relevant are:

• New reference workflow. With the new epi-reference workflow, the contrast of the reference has a little different distribution and I believe the registration parameters are now more sensible (which in the long term is good, but in the short term justifies having bad results).
• Rotation to canonical. For oblique datasets our fieldmap less registration scheme was theoretically flawed because ITK/ANTs work on physical coordinates and the PE axis is therefore not aligned anymore with the data array axis. This may have introduced a strong modification on the workflow (and is prone to bugs).
• I made the tunning with DWI's b-zeros.

In addition to this, I don't remember exactly whether the T1w inversion is enabled by default. I think in the long term it could be disabled when using mutual information, but I don't know what the tool is doing at the moment by default.

Overall, I'll have a deep look into this later this week. I apologize for the disappointment it must be to see this large regression up front.

[oesteban]

Ha, I forgot I had already expressed my concern about the missing PE - #2497.

[oesteban]

I'm picking up on where you left this. The first outstanding problem is that the syn registration was not correctly initialized.

I reorganized the syn workflow in two parts:

• preprocessing
• actually fitting

The idea is that the preprocessing massages the T1w and brings it into rough alignment with the EPI, through an Affine transform. However, for ds005 this registration step was far from acceptable. I am playing now with better parameters that bring the T1w into a seemingly good alignment into the EPI (for subject 02 of ds005). I'll keep playing with this.

[oesteban]

I'm still testing on ds005/sub-01 locally, but this is getting closer to the end line. Results from SDCFlows are starting to seem sufficiently okay to go forward with this PR and separate the problem it its two elements: actual integration (to be addressed within fMRIPrep) and internals and improvement (to be addressed in SDCFlows proper).

I will update as soon as I get new local results.

[oesteban]

My results locally on ds005/sub-01 are very good-looking.

[oesteban]

Okay, once I finalize some work I have optimizing the Docker image build, the next stop is to add a PhaseEncodingDirection and TotalReadoutTime metadata entries on both ds210 and ds005 - WDYT?

[effigies]

+1 to updating PhaseEncodingDirection, though we should also add it to the source datasets on OpenNeuro. We should be able to determine it visually, so I'm comfortable with this.

I don't think adding an arbitrary TRT or EES to the datasets is a good idea. Internally passing around a 1.0 seems fine, but tagging good data with bad metadata would be bad management of these datasets.

[oesteban]

we should also add it to the source datasets on OpenNeuro. We should be able to determine it visually, so I'm comfortable with this.

After some thought, I would discourage doing this. We could add a note to the README or similar, suggesting the setting of the PhaseEncodingDirection for preprocessing purposes, but updating the dataset's metadata might have unforeseen consequences, and it is not reflective of the reality of the dataset - even if effectively we set the right metadata value. OpenNeuro wants to be representative of the data out there, and data without PE direction recorded unfortunately exist out there.

Now, regarding our test datasets, they are just that - test datasets. We should derive them as much as we need to ensure we cover the maximum of the inputs space. I'm happy setting the two metadata values dynamically on every build (that way we can also test that the workflow crashes when not present).

So, let's not modify the metadata anywhere static, WDYT?

EDIT: if we finally modify the README suggesting some PE setting, I'd also make sure that note comes with some sort of admonition stating that doing so should be clearly stated in the methods section of derived works.

[effigies]

we should also add it to the source datasets on OpenNeuro. We should be able to determine it visually, so I'm comfortable with this.

After some thought, I would discourage doing this. We could add a note to the README or similar, suggesting the setting of the PhaseEncodingDirection for preprocessing purposes, but updating the dataset's metadata might have unforeseen consequences, and it is not reflective of the reality of the dataset - even if effectively we set the right metadata value. OpenNeuro wants to be representative of the data out there, and data without PE direction recorded unfortunately exist out there.

Yes, there is data like this out there, but I don't see ON's goal as being representative of the range of data available. When data metadata can be safely improved, that seems fine. We detected L/R flips and fixed them post-hoc. But I'm not going to insist on this.

Now, regarding our test datasets, they are just that - test datasets. We should derive them as much as we need to ensure we cover the maximum of the inputs space. I'm happy setting the two metadata values dynamically on every build (that way we can also test that the workflow crashes when not present).

So, let's not modify the metadata anywhere static, WDYT?

Sounds reasonable.

EDIT: if we finally modify the README suggesting some PE setting, I'd also make sure that note comes with some sort of admonition stating that doing so should be clearly stated in the methods section of derived works.

If the metadata is changed in the dataset, recording the version of the dataset used (and making sure that version has a clear changelog) seems sufficient. If the user modifies the dataset, the methods should reflect that.

[oesteban]

@mgxd @effigies this is ready for merge, I believe. There's one issue though (see #2606) that has held me back to apply the fieldmap estimation to ME data. In other words, in the results for ds210 on Circle you will find that the SyN estimation is executed and produces the expected outputs, but the fieldmaps are not applied.

[effigies]

We'll need to apply to the optimal combination at least. I've got a PR about sinking individual echos. I think we don't want them SDC'd, but should check in with the Tedanites.

[oesteban]

Awaiting #2610 for final reviews.

[effigies]

If PhaseEncodingDirection is found but no source for TotalReadoutTime, then you get a runtime error. If we're not going to impute a default TRT, then we should make this a workflow build-time error.

[effigies]

The final mask doesn't look great on ds005. I will retry with a fresh working directory and report back.

[effigies]

Mask is much improved after clearing the working directory.

[oesteban]

If PhaseEncodingDirection is found but no source for TotalReadoutTime, then you get a runtime error. If we're not going to impute a default TRT, then we should make this a workflow build-time error.

I'm inclined not to impute - adding this to my TODO list.

[pep8speaks]

Hello @effigies! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-10-23 21:10:53 UTC

[oesteban]

Okay, I believe the formal aspect of this PR is finished - i.e., the workflow builds without errors and executes to the end. However, the results are not exciting at all. SDC-SyN performs poorly (esp. if you execute locally), mostly due to #2606 (comment). It is also possible that there are better parameters for ANTs, but I don't think we will reach that point of finesse until we do something about the bold reference in registration.

An additional issue to look at (probably first with our current implementation of ME -without syn-) is that I got the impression that the bold_split node is iterated over an excessive number of times for each of the echos. I will investigate this later today (this could be a result of not joining one field somewhere).

Other than that, the RC will be a notch closer when we merge this in.

[effigies]

Agreed. Let's get this in and work toward further improvements.