ReproNim / reproin

A setup for automatic generation of shareable, version-controlled BIDS datasets from MR scanners
MIT License
47 stars 13 forks source link

heuristic_option: (run) index_format option to allow for non-0-padded run indexes #48

Open dlevitas opened 3 years ago

dlevitas commented 3 years ago

Sorry, this is another instance of me not knowing where to submit the PR request, but I'll describe it here. In the reproin heuristic code there is theiscode snippet:

run_label = "run-" + ("%02d" % current_run
               if isinstance(current_run, int)
               else current_run)

I wanted to see what your thoughts are on removing the zero padding of run labels. From this NeuroStars post, the BIDS spec specifies that run labels should be non-negative integers, and the pre-processed output of newer fMRIPrep version adhere to this integer designation for run labels. Thus, the code above could be changed to:

run_label = "run-" + (current_run
                if isinstance(current_run, int)
                else current_run)
yarikoptic commented 3 years ago

reproin heuristic is developed/shipped within heudiconv, where you reference it from, so any PR should be submitted there

code could not be changed as suggested since you cannot concatenate an int to the string "as is", but I guess you just meant to add it without padding. I like padding since it makes it consistent even if you do end up with >=10 runs which is feasible. It is VERY unlikely anyone would have >=100 runs though. Overall I would be reluctant to change that now.

Although I can possibly see the motivation, it is a bit odd that fmriprep starts to trim those leading 0s in derived output filenames, or may be it is a bug since I would expect fmriprep to preserve entity values as is, @effigies?

effigies commented 3 years ago

It's from shifting to PyBIDS, which parses indices as integers, and does not record padding.

yarikoptic commented 3 years ago

Should then bids recommend to not use zero padding because pybids would remove it for the derived files this making it harder to align, or actually even breaking assumptions for common derivatives that file prefix remains unaltered?

effigies commented 3 years ago

I guess the question should be reopened. I believe most of the discussion has been around the pain of making PyBIDS able to handle something that needs to be treated as both a string and an integer. But that is a solveable problem.

dlevitas commented 3 years ago

It seems that ReproIn will zero pad run numbers, even if it wasn't set in the SeriesDescription. For example, I have an acquisition titled func-bold_task-rest_run-1, but following conversion the filename is sub-01_ses-01_task-rest_run-01_bold.nii.gz. Would it be possible to have ReproIn respect the run entity label provided in SeriesDescription and only zero-pad if the run label isn't specified?

yarikoptic commented 3 years ago

I think that would be good... we can't change right away though since it would make user conversions inconsistent with "upgrade" for no obvious benefit.
In a short run you could get a copy of reproin.py and tune it to your liking.

An alternative is to finally find time to review/finalize or redo https://github.com/nipy/heudiconv/pull/466 to introduce configuration, so then we could have heuristic specific configuration options, and add an option to reproin heuristic (e.g. `index_format="%2d") so it could be tuned up to the user's liking. We already have a good number of similar option ideas identified: https://github.com/ReproNim/reproin/issues?q=is%3Aissue+is%3Aopen+heuristic+option . I will adjust the title of this issue to match.