PennLINC / qsirecon

Reconstruction of preprocessed q-space images (dMRI)
https://qsirecon.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
3 stars 2 forks source link

Issues with Dipy MAPMRI Outputs #54

Closed akimbler closed 2 months ago

akimbler commented 2 months ago

Summary

The MAPMRIOutputSpec has namespace reserved for several MAP-MRI metrics that dipy computes (NG, Parallel NG, Perpendicular NG, and general coefficients) that it doesn't actually provide an output for. It appears that it attempts to save the coeffs inqsirecon/qsirecon/interfaces/dipy.py (see below) but the method is called incorrectly and it just doesn't provide an output.

https://github.com/PennLINC/qsirecon/blob/09951564efebab13542df87e446f1cf71da35d08/qsirecon/interfaces/dipy.py#L294-L295

Additional details

What were you trying to do?

What did you expect to happen?

What actually happened?

Reproducing the bug

tsalo commented 2 months ago

Hey Adam! 👋

I'm working on this in #55. Do you have a reconstruction workflow with anisotropic_scaling: true?

akimbler commented 2 months ago

Hey Taylor! What exactly do you need?

tsalo commented 2 months ago

It looks like the ng, perng, and parng files can't be generated if anisotropic_scaling is set to false in the MAPMRI_reconstruction node of the reconstruction workflow, and none of the built-in ones have it set to true. If you have a reconstruction workflow that does have it set to true, I was hoping you could test my branch locally and see if the results look good to you. Otherwise, I can modify one of the existing tests, but I don't know how anisotropic_scaling would affect the results.

akimbler commented 2 months ago

@tsalo I just double-checked the MAPMRIInputspec, it seems like it's set to default to anisotropic_scaling=True, so it seems like there's an inconsistency somewhere

tsalo commented 2 months ago

I think all of the built-in workflows (dipy_mapmri.json and multishell_scalarfest.json) explicitly set it to False though.

akimbler commented 2 months ago

I'll grab your branch and give it a test as soon as I can, I'm confident it'll run. Is there a reason that we know of that the built-in from qsirecon would have defualts that differ from the dipy defaults? It's 100% set to true by default from dipy proper https://github.com/dipy/dipy/blob/1cc364f2b20479b29063dd7ed231d9ab3fcd710f/dipy/reconst/mapmri.py#L90

tsalo commented 2 months ago

That's a question for @mattcieslak. He told me his reasons on Slack, but they went over my head.

mattcieslak commented 2 months ago

There was a paper awhile ago that showed that (IIRC) anisotropic scaling was bad for tractography but good for estimating propagator measures. Back when I wrote these workflows I was using the MAPMRI fits to estimate ODFs for tractography so I had turned it off.

akimbler commented 2 months ago

Should either the dipy_mapmri.json and multishell_scalarfest.json be set to dipy defaults or should at least MAPMRIInputSpec be changed to be consistent then? I was a little shocked because I'm doing the exact opposite and I'm working with propogator estimates.

mattcieslak commented 2 months ago

I think definitely for multishell_scalarfest.json we should change it to the dipy default. For dipy_mapmri.json we can add a note to the documentation to change this parameter depending on your goals.

tsalo commented 2 months ago

I didn't change the recon configs, so that'll have to happen in a separate PR.