PeerHerholz / bids_bep16_conv

A small package to implement conversions between BEP-16 (Diffusion Derivatives) compliant datasets and other/existing software outputs.
https://peerherholz.github.io/bids_bep16_conv
BSD 3-Clause "New" or "Revised" License
0 stars 2 forks source link

.mif conversion #10

Open Lestropie opened 1 year ago

Lestropie commented 1 year ago

Currently, in order to achieve conversion between MRtrix and BEP016, I would need to be able to invoke MRtrix's mrconvert command. This may result in an implementation that deviates quite a bit from other conversion functions, in that it would use subprocess and potentially need a container recipe.

It would be preferable to instead have .mif / .mih support in nibabel: https://github.com/nipy/nibabel/issues/927

arokem commented 1 year ago

What are your thoughts on https://github.com/dchristiaens/mrtrix3-pyio (mentioned in that issue)? Sadly, no license in sight.

Lestropie commented 1 year ago

I still have never looked at it. But even @dchristiaens agreed at some point during discussion that nibabel integration would be preferable long-term. There was some effort invested in the latter by @kitman at https://github.com/kitman98/nibabel, but I've not had a chance to see how much effort is required to get it to the point of a PR.

arokem commented 1 year ago

Oh - that looks pretty well-advanced. Seems like a promising direction.

Alternatively, mrtrix could be rewritten to produce nifti files :-)

Lestropie commented 1 year ago

MRtrix already reads and writes NIfTIs for any command; always has. Many other formats also. And it's all handled by the back-end API.

But if you want a conversion tool that "takes as input an MRtrix output and produces BEP016-compliant data", the input data are more likely to be provided as .mif's, so it makes sense for the conversion tool to be capable of reading such, rather than demanding that a user produce / convert to NIfTIs before executing that tool. Also, if the data are in .mif / .mih, the conversion tool could potentially interrogate the command provenance stored in the header and infer certain BEP016 metadata fields based on the usage that led to those data.

arokem commented 1 year ago

That makes a lot of sense!

PeerHerholz commented 1 year ago

Hi @Lestropie & @Lestropie,

MRtrix was added to the container via this commit and is thus available, including mrconvert. However, this only holds true for the container version of the package and we would need to change the requirements for the bare metal version/tests to use an environment.yml instead of a requirements.txt given that MRtrix has to be installed via conda. Please let me know if I should implement this respectively.

Cheers, Peer

Lestropie commented 1 year ago

We can stick with the MRtrix mrconvert dependency for now. For outside of the container, rather than demanding conda it could just require that mrconvert be available in PATH. But longer term it'd be preferable to remove that dependency.

PeerHerholz commented 1 year ago

Ok, thanks.

Just as a follow-up on your initial comment: the DIPY workflows also use subprocess atm, e.g. dipy_fit_DTI. Thus, I wouldn't consider this a "problem" for MRtrix.

Additionally, if we intend this to be a BIDS App, then its most common installation/application would be via the container anyway, no? Therefore, mrconvert could stick around for a while and we can adapt once there's something else that works.

For outside of the container, rather than demanding conda it could just require that mrconvert be available in PATH.

So, the idea would be to denote this as a dependency that needs a bit more attention and would need to be set up respectively, right? If so, I can add this to the docs.

Lestropie commented 1 year ago

the DIPY workflows also use subprocess atm, e.g. dipy_fit_DTI. Thus, I wouldn't consider this a "problem" for MRtrix.

That sounds like a case of "processing" rather than "conversion", if it's doing a "fit". In the former case, obviously MRtrix3 binaries are going to be necessary over and above just mrconvert. Hence why I wanted clarity on #7. It's only if an app were restricted to "conversion" that .mif support in Python would be beneficial.

PeerHerholz commented 1 year ago

Yeah, right now we need the processing part in order to test and implement the conversion part. If the app is changed to a "pure conversion" tool at a later point, then this could be adapted respectively. However, as it is intended to run as a BIDS-App in container form, we could leave it for the time being and I would update/adapt the tests via adding a conda install mrtrix3 -c mrtrix3 command to get the respective environment needed there.