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

dataTypeIndex is defined ambiguously #87

Closed
MichaelUM opened this issue Sep 28, 2021 · 4 comments · Fixed by #152
Closed

dataTypeIndex is defined ambiguously #87

MichaelUM opened this issue Sep 28, 2021 · 4 comments · Fixed by #152
Assignees

Comments

@MichaelUM
Copy link

The dataTypeIndex is not clearly defined. In the data format summary it is defined as an <i> integer but in the text description it states Data-type specific parameter indices and Note that the Time Domain and Diffuse Correlation Spectroscopy data types have two additional parameters and so the data type index must be a vector with 2 elements that index the additional parameters.
Also here the type needs to be changed from Type: integer to Type: numeric 1-D array.

@samuelpowell
Copy link
Collaborator

Following meeting of May 24 it was agreed that dataTypeIndex should be a scalar which is used as an index into all appropriate fields. Indeed, it appears that this was the original intent since fields such as /nirs(i)/probe/timeDelays state, for example, that the "indexing of this field is paired with the indexing of probe.timeDelayWidths".

The fix here is to rewrite the description and check that all fields for which the same index might be used contain similar text to that above, or at least, nothing contradictory.

@HanBnrd
Copy link
Member

HanBnrd commented Jul 20, 2024

Hi, do you mean its type should be changed to [<i>,....], and it should be renamed to dataTypeIndices? And therefore the way it is used should change too, for example here:

For example, if measurementList5 is a structure with sourceIndex=2, detectorIndex=3, wavelengthIndex=1, dataType=1, dataTypeIndex=1 dataTypeIndices=[1] would imply that the data in the 5th column of the dataTimeSeries variable was measured with source 2 and detector 3 at wavelength 1.

@samuelpowell
Copy link
Collaborator

The intent is that dataTypeIndex remains a scalar, and that the rest of the specification is updated to make this consistent. First, the potential for a vector form should be removed from the description:

Note that the Time Domain and Diffuse Correlation Spectroscopy data types have two additional parameters and so the data type index must be a vector with 2 elements that index the additional parameters

Second, each of the variables that are indexed by dataTypeIndex should be checked for consistency. For example, timeDelays is consistent:

This field describes the time delays (in TimeUnit units) used for gated time domain measurements. This field is only required for gated time domain data types, and is indexed by measurementList(k).dataTypeIndex. The indexing of this field is paired with the indexing of probe.timeDelayWidths.

Note here that it explicitly states that the same single index is used to access both timeDelays and timeDelayWidths. This implies some redundnacy in the arrays, because there will need to be a timeDelayWidths elemt for every timeDelays, but this is the decision that has been taken to maintain simplicity.

Third, the document should be scanned for any other references to this field to ensure that any descriptive text is consisten with the description of this field as a scalar. Note that in addition to searching for the field name itself, text such as 'data type' should be checked, as there are some peieces of descriptive prose.

@HanBnrd
Copy link
Member

HanBnrd commented Jul 22, 2024

Alright, thanks for the details @samuelpowell, I'll open a PR.

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 a pull request may close this issue.

3 participants