[FIX] Clarify indices are nonnegative integers.

[nicholst]

At present, zero is not excluded for run and echo indices. Is this accidental or intended?

If zero's are excluded, this PR will make this explicit.

If zeros are to be discouraged, then perhaps we should say so?

[nicholst]

Sorry! I jumped too quick! @effigies points out that natural numbers are not unambiguously defined.

I've updated this to replace natural number with positive integer. It's a small difference, but, again, it seems we should be explicit as to whether 0's are allowed or not, or if allowed if they're discouraged.

[sappelhoff]

I think it'd be good to exclude 0 as an index value explicitly (From my feeling this is already implicit in the spec).

But an edit would probably best be made under point 9. here: https://github.com/bids-standard/bids-specification/blob/master/src/02-common-principles.md#definitions

and then we could reference to this definition from elsewhere in the spec.

[effigies]

I feel this is an unnecessary change. The most common use case will be sequential indices, starting at 1, but it's not clear why we should rule out other patterns, such as 0-indexing. run-0 seems perfectly interpretable to me, and software would need to do more work to find run-1 but not run-0 than finding both.

My vote would be leaving it as it is. If the collective does decide to change it, though, I think we should change all indices everywhere, including in the definitions, as Stefan notes. And we'll need to figure out a good English version of /0*[1-9][0-9]*/ as opposed to "numeric" which translates to /[0-9]+/.

[nicholst]

Ok, here’s the converse: is the spec clear that a *negative* index is disallowed? I know it’s obviously stupid, but does the text and/or logic of the spec rule out run--1? --

…

_________________________________________________________ Thomas Nichols, PhD Professor of Neuroimaging Statistics Nuffield Department of Population Health | University of Oxford Big Data Institute | Li Ka Shing Centre for Health Information and Discovery Old Road Campus | Headington | Oxford | OX3 7LF | United Kingdom T: +44 1865 743590 | E: [email protected] W: http://nisox.org | http://www.bdi.ox.ac.uk

[effigies]

Yes. Numeric characters are just [0-9], so negative numbers aren't possible.

[nicholst]

Ok, as long as it’s unambiguous I’m fine to close this. My only remaining question is if we want to explicitly discourage indexing from zero. While I agree it’s elegant to allow zeros, if someone later is writing code to aggregate 1000’s of BIDS datasets, wouldn’t it be a headache if a random sprinkling started indexing from zero while other started from 1?

…

--

_________________________________________________________ Thomas Nichols, PhD Professor of Neuroimaging Statistics Nuffield Department of Population Health | University of Oxford Big Data Institute | Li Ka Shing Centre for Health Information and Discovery Old Road Campus | Headington | Oxford | OX3 7LF | United Kingdom T: +44 1865 743590 | E: [email protected] W: http://nisox.org | http://www.bdi.ox.ac.uk

[sappelhoff]

While I agree it’s elegant to allow zeros, if someone later is
writing code to aggregate 1000’s of BIDS datasets, wouldn’t it be a
headache if a random sprinkling started indexing from zero while other
started from 1?

I have no help on this (50% / 50% opinion currently), but this may be related to #499

[effigies]

It's not obvious to me that it would be a problem, as I wouldn't expect run indices to form a meaningful grouping across datasets, but I don't mind RECOMMENDing that indexed entities be sequential and start from 1.

[nicholst]

Good call @effigies ... and I've updated the title and content of this PR to simply add the text indexing starting from 1 RECOMMENDED in the definition of run and echo.

[nicholst]

Oh dear... I ran git rebase master on my branch, so that when I personally run git diff master I only see changes on the two lines I edited, but now it appears that this PR has both this tiny change and changes from #536 (replace non-breaking spaces).

If someone knows what I need to do to fix this please let me know... sorry!

[effigies]

Looking at the history, you appear to have run git pull after rebasing, which is probably because you tried git push and it said you needed to pull first. I would suggest rebasing again (selecting 51bb58d, 7202175 and d678512), and then git push --force.

I tried closing and reopening to see if that would fix the history, but it did not. If you set the flag that maintainers can edit your PR, I may be able to push a fixed branch, if it doesn't work out for you.

[nicholst]

Thanks @effigies ! But I got as far as git rebase 51bb58d and was told I had a slew of conflicts... not sure what to do.

On my page "Allow edits and access to secrets by maintainers" is checked so you should be able to mess with it.

[effigies]

Force-pushed.

Just to document what I did, in case it's helpful:

git checkout NoZeroRun
git rebase -i upstream/master

Which opened up a VIM:

pick 51bb58d Rule-out 0 as an index value
pick 7202175 Replace 'natural number' with 'positive integers'
pick b16cd4b Rule-out 0 as an index value
pick b955b7b Replace 'natural number' with 'positive integers'
pick d678512 Make 1-indexing recommended

Removed the duplicates, saved and closed:

pick 51bb58d Rule-out 0 as an index value
pick 7202175 Replace 'natural number' with 'positive integers'
pick d678512 Make 1-indexing recommended

Which showed:

Successfully rebased and updated refs/heads/NoZeroRun.

Then:

git push -f

[nicholst]

Ah! Thank you Chris!

…

[sappelhoff]

Thanks, changes look good to me. I merely added some line breaks where I thought they'd make sense

(and with "sense" I mean --> wherever I thought they may improve a future git diff, if we ever touch these lines again)

[nicholst]

Thanks *@sappelhoff!* *I had understood we do line breaks only at the end of sentences. * --

…

_________________________________________________________ Thomas Nichols, PhD Professor of Neuroimaging Statistics Nuffield Department of Population Health | University of Oxford Big Data Institute | Li Ka Shing Centre for Health Information and Discovery Old Road Campus | Headington | Oxford | OX3 7LF | United Kingdom T: +44 1865 743590 | E: [email protected] W: http://nisox.org | http://www.bdi.ox.ac.uk

[sappelhoff]

*I had understood we do line breaks only at the end of sentences. *

Yes, sorry for the misunderstanding --> We are not very strict about it, my heuristics are:

• don't make the lines too long
• always start a new line after the end of a sentence
• if there is a natural break point within a sentence, use that to put a line break there
• don't forget the goal: good readability for humans with and without diff rendering

[emdupre]

Thanks for bringing this up, @nicholst ! I'm lightly against this change, but I want to preface by saying I could easily be convinced 😸 I'm just not sure that I see the need for this yet.

To give a bit of context, a while back we had suggested that echo indices be required to sequentially increase with increasing echo times, but this was recognized as backwards incompatible and delayed to BIDS 2.0. This is actually something that BEP001 just had to downgrade to a recommendation for exactly this reason.

In developing tools for multi-echo data, then, we hit a bit of a wall in that -- although existing publicly-available multi-echo data sets start at 1 and use increasing indices -- tool developers still can't be sure that a given dataset is indeed using increasing indices. So, the recommendation doesn't help us directly in practice.

All that to say, so long as it's clear that we can't assume that data sets will actually follow this recommendation, then it's fine with me. But I worry somewhat that if this is included in the spec, tools developers or users might assume that BIDS compliant data sets start at one (and then, effectively, that echo indices increase sequentially), when we can't guarantee that to be true.

[satra]

i think (but could be convinced otherwise as well) that i agree with @effigies and @emdupre that changing this to a recommendation is ok but does not offer any obvious advantage over leaving it as is.

let me offer an example where the indexing may be used as semantics. i have an fmri study where i randomize a set of stimulus patterns. nothing in bids, says at this point that my run indices have to be in order of acquisition.

so i could reorder every participant in a consistent pattern of runs to correspond to a fixed ordering of the paradigm files (*events.tsv). i can use inheritance to make this common across every participant. further, for some reason (e.g. lack of scanning time, bad acquisition, etc.,.) the task run-1 or run-N could be missing. i still satisfy all of bids requirements and have reduced the possibility that different events.tsv are erroneous to a limited error surface.

given that it's a recommendation, the proposed change won't prevent one from still doing this, but the proposed changed may suggest to folks that a pattern starting from 1 should be catered for. instead the more general and current specification simply says the index is an integer and allows for flexibility of use. and it's left up to a tool developer to sort it and extract metadata as necessary. and thus any tools catering to the present specification is likely to be more generally applicable to numeric indexing, whether numerically sequential or not.

[nicholst]

First let me say I’m not on a 1-indexing rampage, but rather trying to put myself into the shoes of someone new to BIDS trying to do the ‘right’ or best practice thing, and it seemed strange that the standard had no steer on 0- vs 1-indexing conventions. Also, I didn’t intended to add text (and I don’t see any text) that suggests sequential indexing is requisite. (Though the examples offered suggest sequential indexing from 1). So... anyway, I’m not trying to “change” anything, but if there is something implicit that everyone assumes it is probably worth making explicit, even if that is just a RECOMMENDATION.

[satra]

@nicholst - i was getting at the notion that any pattern is ok. so perhaps i should instead ask, what does starting with 1 help with?

[nicholst]

Sorry @emdupre, I skipped over your comment, and the full meaning of @satra's. If you think the recommendation will lull developers to expect only 1 (and, worse, only sequential ordering), I can see the harm of the recommendation.

Anyway, I was about to push a change to remove the RECOMMENDED text to discover that, here on this remote island, VPNs and ssh-ing is blocked! I'll update this when I have a chance but it might not be for another 2 weeks.

Happy to hear any other comments in the mean time.

[nicholst]

Have now pared down the PR to remove any recommendation of 0- or 1-indexing.

The change now amounts to nothing more than specifying that only nonnegative integers are permitted as run and echo labels, which is strictly redundant given that BIDS <label> is defined to be "an alphanumeric value" in Common principles, but I think it is a clarification worth adding.

[emdupre]

This looks good to me. Thank you, @nicholst !

[sappelhoff]

Thanks @nicholst and reviewers!