nipy / heudiconv

Flexible DICOM conversion into structured directory layouts
https://heudiconv.readthedocs.io
Other
232 stars 125 forks source link

populate_intended_for prefers `task` entity with `CustomAcquisitionLabel` method #767

Closed octomike closed 2 months ago

octomike commented 2 months ago

Summary

Apparently heudiconv prefers to use the task entity when trying to match fmaps to funcs.

elif matching_parameter == "CustomAcquisitionLabel":
        modality = op.basename(op.dirname(json_file))
        if modality == "func":
            # extract the <task> entity:
            custom_label = BIDSFile.parse(op.basename(json_file))["task"]
        else:
            # extract the <acq> entity:
            custom_label = BIDSFile.parse(op.basename(json_file))["acq"]

While this is initially unintuitive (the option is called CustomAcquisitionLabel after all) I found it's also problematic when there is more than one task. How would I match a single fmap to multiple functionals here?

Wouldn't it be better to try a shared acq label first and then only fall back to the task entity?

Platform details:

Choose one:

yarikoptic commented 2 months ago

apparently this was added in 784e946af417955aa0c3515e3f4053bc1a274413 all the way in

I think we could add AcquisitionLabel or PlainAcquisitionLabel which would be making decision solely based on the _acq-<label>. Care to submit a PR adding it (just git grep CustomAcq to see all spots to change)?

octomike commented 2 months ago

Happy to.

I didn't duplicate create_dummy_no_shim_settings_custom_label_bids_session, because I thought it's a little over the top. Do you want me to though?

octomike commented 2 months ago

Is there anything I should add here? Throw it all away and start over? :)

yarikoptic commented 2 months ago

Addresses by #768