-
Notifications
You must be signed in to change notification settings - Fork 45
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
0- vs 1-indexing the run
keyword in use_heuristic()
#89
Comments
I see that actually we were talking about two different things. As we don't support multiple files or multiscan files yet, if we want to implement this we should also think about a way to check that then different calls of phys2bids respect the overall indexing, i.e. if the user adopts a 0-indexing and has -0 -1 -2 we don't overwrite -1. |
@rmarkello let's get back to this issue. Shall we agree on suggesting the user to adopt a 1-indexed run keyword, and remain agnostic about it? |
I am for starting at |
Do you think we need to add something to the code as it is now to accommodate for this decision? |
It looks like there are a few instances in the heuristic files provided with phys2bids/phys2bids/heuristics/heur_test_acq.py Lines 19 to 22 in 3dc8c8a
phys2bids/phys2bids/heuristics/heur_tutorial.py Lines 19 to 22 in 3dc8c8a
These heuristics and all relevant tests / documentation would need to be updated to reflect this decision. |
That's right - actually, those values are just an example. We can definitely change them without any big impact! |
In #76 (comment) a brief discussion was started about whether the
run
keyword should be 0- or 1-indexed when users provide aheuristic
to convert their data to BIDS format. We are currently using a 0-indexing approach; I personally think a 1-indexing approach makes more sense here, but am open to discussion on this point!@smoia found a relevant portion of the BIDS spec that touches on this, but the spec is not prescriptive on this point. I'm happy to hear others thoughts and go about issuing a change if it's decided that's warranted!
The text was updated successfully, but these errors were encountered: