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

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

Open
dlevitas opened this issue Feb 15, 2021 · 6 comments

Comments

@dlevitas
Copy link

dlevitas commented Feb 15, 2021

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
Copy link
Member

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
Copy link

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

@yarikoptic
Copy link
Member

yarikoptic commented Feb 16, 2021

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
Copy link

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 dlevitas changed the title Make run ID non-negative integer Make run ID non-negative integer? Feb 16, 2021
@dlevitas
Copy link
Author

dlevitas commented Feb 16, 2021

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 yarikoptic changed the title Make run ID non-negative integer? heuristic_option: (run) index_format option to allow for non-0-padded run indexes Feb 16, 2021
@yarikoptic
Copy link
Member

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 nipy/heudiconv#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.

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

No branches or pull requests

3 participants