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

Clarification that aux is not limited to the supported fields #86

Closed
wants to merge 2 commits into from

Conversation

MichaelUM
Copy link

Updated definition of the /nirs(i)/aux(j) field to make it more clear that not only supported entries are allowed.

See #81 (comment)

Updated definition of the /nirs(i)/aux(j) field to make it more clear that not only supported entries are allowed.

See fNIRS#81 (comment)
Changed additional to more
@sstucker
Copy link
Collaborator

sstucker commented Sep 28, 2021

Why don't we use language like "while a list of standard names is specified in appendix, /nirs(i)/aux(j).name can take any value."

Also, update the CHANGELOG with your fix.

@MichaelUM
Copy link
Author

Then I would in general remove the term "supported" for aux because, if I understood it correctly, for the datatypelabel only specific entries are allowed, but this is also stated as "supported" in the appendix. (Also here it states "possible" values)

@sstucker
Copy link
Collaborator

if I understood it correctly, for the datatypelabel only specific entries are allowed

Yeah, it does read like this, but probably that shouldn't be the case either. The intention is only to provide some standard values for users to settle on to describe common types.

@dboas do you agree that dataTypeLabel CAN take any value? People will come up with their own processed types in the future, probably, while we encourage using the standard list we will add to the table in the appendix.

@dboas
Copy link
Collaborator

dboas commented Sep 28, 2021

Looks to me that this PR doesn't state it correctly. @sstucker says it the way I think we want it to be said, that the dataTypeLabels and aux.names indicated in the appendix are suggested values to use in specific circumstances to facilitate the sharing of code to properly interpret these fields in data provided by different sources. But people are free to use what ever values they want although then it reduces the ease with which those values can be analyzed by other code. We encourage people who have need to add new values to make suggestions to add those values to the snirf spec.

Can you make it say something like that?

@MichaelUM
Copy link
Author

@sstucker @dboas thanks for the clarification. I will change the PR accordingly and also integrate the change for the other fields. Sorry for nitpicking...

@sstucker
Copy link
Collaborator

Not nitpicky at all, it is great this stuff is being ironed out. Thanks for this contribution

@sstucker
Copy link
Collaborator

Bumping this in case its something we still want to do @MichaelUM

@sstucker
Copy link
Collaborator

sstucker commented Jun 1, 2022

Implemented in #116

@sstucker sstucker closed this Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants