Event onsets MUST NOT be negative

[yarikoptic]

I found not a single openneuro datasets with negative onsets.

smaug:/mnt/datasets/datalad/crawl/openneuro
*$> for ds in ds*; do git grep '^ *-' '**_events.tsv' | head ; done

But ATM we are looking at a dataset with some buggy _events.tsv which has huge negative values sorted within onsets column. I think in general/theory in some .1% of the cases there could be point to have onsets with negative onsets, e.g. due to desire to encode some preparatory to the data scanning activities (e.g. during dummy scans etc). So for those -- it would be nice to have a dedicated ERROR code to ignore/overwrite. But in general, it just signals a problem with the file and thus IMHO should be an ERROR.

TODOs (@effigies @rwblair @tsalo - please guide)

• how to handle n/a values there? (should be ok)
• how/where to add a test?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.58%. Comparing base (1e99fc3) to head (00be46e).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2075   +/-   ##
=======================================
  Coverage   82.58%   82.58%           
=======================================
  Files          17       17           
  Lines        1499     1499           
=======================================
  Hits         1238     1238           
  Misses        261      261           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rwblair]

- expression: min([-1, "n/a", 1])                                                                                        
  result: -1

In the expression_tests.yaml negative values are treated as less than "n/a". I believe the deno validator handles this case in its implementation.

[Remi-Gau]

that would be breaking backwards compatibility, no?

for as far as I can remember negative onsets have been explicitly allowed: https://bids-specification.readthedocs.io/en/stable/glossary.html#onset-columns

Description: Onset (in seconds) of the event, measured from the beginning of the acquisition of the first data point stored in the corresponding task data file. Negative onsets are allowed, to account for events that occur prior to the first stored data point. For example, in case there is an in-scanner training phase that begins before the scanning sequence has started events from this sequence should have negative onset time counting down to the beginning of the acquisition of the first volume.

[effigies]

This would be a breaking change:

https://bids-specification.readthedocs.io/en/stable/modality-specific-files/task-events.html

Column name	Level	Data type	Description
onset	REQUIRED	number	Onset (in seconds) of the event, measured from the beginning of the acquisition of the first data point stored in the corresponding task data file. Negative onsets are allowed, to account for events that occur prior to the first stored data point. [...]

There's very little point in including an onset <-10s. We could set a threshold at, say, -30s or -60s to allow for unconsidered cases while identifying probable errors.

If we are going to set a lower bound in the schema, it should also be indicated in the text.

[yarikoptic]

d'oh @effigies , and I thought I checked for that being spelled out!

I am Ok with making it "up to 60 seconds" or any other reasonable amount for an ERROR. I wonder if it would still make sense to issue a warning if any negative is encountered given that none of the datasets seems to use them -- WDYT?

[Remi-Gau]

Personally I'd prefer to be conservative not make it an error but just a warning as suggested by @VisLab
#2075 (comment)

[yarikoptic]

ok, made it into a warning and extended description -- or would such wording be not advisable there?

[yarikoptic]

ok, got "inspired" and added one for implausible positive one. I think not even .1% of users should stick more than a month worth of events into one file. WDYT?

[VisLab]

I'm not sure these checks are necessary. On the negative onsets, I think I have seen driving simulation experiments where there were simulator events and driver response events that were recorded in the events file before the EEG recording was turned on after the person was driving a while. The behavioral events were recorded through the whole thing, but the onset was zeroed at the point the EEG started.

On the upper end... I am not sure why this would be necessary either. Consider a wearable EKG recording --- it is possible to have long term monitoring....

[yarikoptic]

They are necessary as I think they could help catch buggy data, as happened in our case and was identified only thanks due to HED validation! ;) (just a side effect)

The use cases where such onsets would be legit would constitute probably less than .1% of the studies given that I found none among existing openneuro for negative onsets altogether. In those few cases, it would be easy to add ignore for those warning codes and sleep happily after.

Even with EKG recording -- I think it is very unlikely (is there a single instance anywhere?) to have over a month long (I was very liberal) recording.

[Remi-Gau]

Should the definition of onsets be amended to mention those warnings?

[yarikoptic]

Should the definition of onsets be amended to mention those warnings?

Lazy me: I think they are of the kind of SUSPICIOUSLY_LONG_EVENT_DESIGN and SUSPICIOUSLY_SHORT_EVENT_DESIGN hint types.. I do not think we document for them.

Doer me: I pushed change to use SUSPICIOUS_ instead of IMPLAUSIBLE_ prefix here for consistency