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

Remove incomplete local channel indexing #151

Merged
merged 3 commits into from
Aug 10, 2024
Merged

Remove incomplete local channel indexing #151

merged 3 commits into from
Aug 10, 2024

Conversation

samuelpowell
Copy link
Collaborator

The current implementation of local channel indexing is incomplete, because there is no defined mapping from local indices to global indices.

We further note that all practical purposes (e.g. analysis) a global index is required.

It is recognised that local indices do provide additional information. For example, there may be differing noise characteristics in the data which are dependent upon the local indices. However, capturing this information by means of indexing is vendor specific, and exploitation requires an intimate knowledge of the system.

If it is decided that such information should be retained, this should be done in a vendor agnostic and explicit manner. For example, in the case of noise characteristics, a per-channel noise distribution could be specified. But more generally, further design is required to understand how to most flexibilty translate the qualities of the acquistion system to the analysis phase, consider e.g., #141.

Thus, on balance, it has been decided to remove the current local indexing implementation.

We believe that the lack of specificaion of a mapping from local to global indexing means that it is not possible to have produced a compliant SNIRF file using local indexing, and hence this change is non-breaking.

Fix #106.

@dboas
Copy link
Collaborator

dboas commented Aug 10, 2024

@rob-luke and @sstucker
can you two review this pull request ASAP!

Your review has been requested and other urgent things are waiting for this pull request to be approved.
Thanks

Copy link
Collaborator

@sstucker sstucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that this requires a validator update once released.

@samuelpowell samuelpowell merged commit 585e2e0 into master Aug 10, 2024
4 checks passed
sreekanthkura7 added a commit to sstucker/snirf that referenced this pull request Aug 16, 2024
Remove following fields that are removed in PR fNIRS#151.
data.measurementList.moduleIndex
data.measurementList.sourceModuleIndex
data.measurementList.detectorModuleIndex
probe.useLocalIndex
@HanBnrd HanBnrd mentioned this pull request Oct 10, 2024
@HanBnrd HanBnrd deleted the nolocal branch October 10, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local to global indexing
3 participants