Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Append dcm2niix postfix to acq only if acq is not defined #214

Closed

Conversation

dzemanov
Copy link

I suggest not appending to acq label dcm2niix postfix if user has defined the acq label in bidsmap, since that is the acq label user needs.

@marcelzwiers
Copy link
Collaborator

marcelzwiers commented Dec 21, 2023

I'll look into this, but I'm sick now, then there is x-mas + new year, so I'm afraid it won't be quick :-(

@marcelzwiers
Copy link
Collaborator

So if not in the acq-label, where do you want to store the string that was appended by dcm2niix? You can't just throw it away, it contains meaningful information (such as coil number, echo number, etc)

@dzemanov
Copy link
Author

No worries, thank you for the quick response!

I understand your concerns, you are right that the information is important.

The reason why I suggested this change is that we have some datasets that are concretely mapped with defined acq label and we would like to use that one, however, not handled dcm2niix postfixes are appended to it as well.
I could create a script that just strips away appended dcm2niix postfixes, but I am not sure if I can correctly identify them with regex and not strip away by mistake part of our acq label. The script would then need to handle bidsname change regarding incrementing runindex and propagating changes to IntendedFor field, scans...

We have this problem with appended phMag postfix, which could be relatively easily stripped. I'm considering whether it's actually redundant, given that similar information might be available in the 'ImageType' attribute (specifically, the 'P' and 'M' indicators). However, I'm not entirely certain about this redundancy. Or if additional steps need to be made for the file to be BIDS valid, since I found only part-phase and part-mag BIDS examples.

Another postfix, _iN, presents a challenge. I think automatically removing it might lead to errors.

A potential solution could be to introduce a unique keyword in BIDScoin naming convention of acq label signaling the start of any dcm2niix-generated postfixes. This way, we can identify what part of acq label is our and what part is generated by dcm2niix.

An alternative approach could be to store dcm2niix postfixes in bids_mappings.tsv file from #213 if it gets merged. In this file, we could add a new column titled 'dicom2niix postfixes' where we list all the postfixes generated by dcm2niix per output file, so the information won't be lost.

For echo number, I thought this gets extracted to echo label, is there a case when echo is extracted to acq label?

I am relatively new to this, the old scripts we used for bids conversion just threw away iN image number or phMag, so files would get incremented run index if needed. Is there a defined way how to handle these cases?

@marcelzwiers
Copy link
Collaborator

How about making it an dcm2niix2bids option? For instance adding a field like: {fallback: acq}. You can leave that empty if needed

@marcelzwiers
Copy link
Collaborator

marcelzwiers commented Jan 11, 2024

I made an effort to store unintentional dcm2niix suffixes in the appropriate BIDS entities, but if there isn't any (e.g. coil number), only then I store it in the acq label. But that should really be the last resort, if nothing else works

@marcelzwiers
Copy link
Collaborator

For the echo number, yes, certain sequences can produce multi echo data, but then there isn't always an echo entity in BIDS for it

@dzemanov
Copy link
Author

How about making it an dcm2niix2bids option? For instance adding a field like: {fallback: acq}. You can leave that empty if needed

That would be amazing!

@marcelzwiers
Copy link
Collaborator

marcelzwiers commented Jan 15, 2024

Mhh, our commits crossed each other, I just fixed it as well :-)
See commit efcd7ac

@dzemanov
Copy link
Author

Yes, apologies, I didn't notice. Yours is much better! Thank you very much for looking into this.

@marcelzwiers
Copy link
Collaborator

Thanks for your PR, please re-open if it doesn't work (I didn't test it :-))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants