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

/nirs(i)/stim(j)/data does not use TimeUnit #111

Open
rob-luke opened this issue Apr 27, 2022 · 2 comments
Open

/nirs(i)/stim(j)/data does not use TimeUnit #111

rob-luke opened this issue Apr 27, 2022 · 2 comments
Labels
breaking A change that would break backwards compatibility with previous releases of the specification v2.0 Things to be considered for version 2

Comments

@rob-luke
Copy link
Member

The specification uses the field TimeUnit to specify the unit that times are stored in. This field is used in many places including /nirs(i)/data(j)/time, /nirs(i)/probe/timeDelays, /nirs(i)/aux(j)/time etc.

However, the /nirs(i)/stim(j)/data field specifies that seconds must be used. It states (accentuation my own)...

This is a numeric 2-D array with at least 3 columns, specifying the stimulus time course for the jth condition. Each row corresponds to a specific stimulus trial. The first three columns indicate [starttime duration value].
The starttime, in seconds, is the time relative to the time origin when the stimulus takes on a value; the duration is the time in seconds that the stimulus value continues, and value is the stimulus amplitude. The number of rows is not constrained. (see examples in the appendix).

This seems counterintuitive to me. Ideally the same time unit would be used all throughout the specification. Unfortunately, changing this would be a breaking change. So I am tagging this as "breaking change" and "Version 2" so that we track this inconsistency and can possibly fix it in the future.

@rob-luke rob-luke added breaking A change that would break backwards compatibility with previous releases of the specification v2.0 Things to be considered for version 2 labels Apr 27, 2022
@rob-luke
Copy link
Member Author

As noted in mne-tools/mne-python#10538 (comment)

@samuelpowell
Copy link
Collaborator

samuelpowell commented May 29, 2024

Reviewed in meeting of May 24 and confirmed that this change should be implemented in v2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A change that would break backwards compatibility with previous releases of the specification v2.0 Things to be considered for version 2
Projects
None yet
Development

No branches or pull requests

2 participants