sofacoustics / SOFAtoolbox

SOFA Toolbox (API for Matlab, Octave)
http://sofaconventions.org
European Union Public License 1.2
116 stars 31 forks source link

Incorrect data type in AnnotatedAudio Conventions? #78

Closed f-brinkmann closed 11 months ago

f-brinkmann commented 1 year ago

@petibub @isfmiho I started integrating the conventions into sofar and I think the AnnotatedAudio conventions might have a problem with the Response* fields:

Name Default Flags Dimensions Type Comment
Response I, C, S attribute the subject’s response
Response:Type I, C, S attribute type depends on the dimension
Response:LongName S attribute narrative description of the response type

My assumption is

isfmiho commented 1 year ago

Hi @f-brinkmann,

@petibub: Do you agree?

petibub commented 1 year ago

Yes. Attributes have no dimensions. Type of Response should be string, with {''} as default, shouldn't?

f-brinkmann commented 1 year ago

We discussed that it should be the following:

Name Default Flags Dimensions Type Comment
Response I, C, S string the subject’s response
Response:Type attribute type depends on the dimension
Response:LongName attribute narrative description of the response type
f-brinkmann commented 12 months ago

@isfmiho I prepared the changes in sofar and sofa_conventions. Can you ping me if the convention is updated? I will then check it for consistency and add it to sofar

isfmiho commented 12 months ago

I updated the table regarding the string's default value to the table below.

@f-brinkmann can you please test _AnnotatedEmitterAudio0.2 and _AnnotatedReceiverAudio0.2 from the development branch!?

Name Default Flags Dimensions Type Comment
Response  {''}   I, C, S string the subject’s response
Response:Type       attribute type depends on the dimension
Response:LongName       attribute narrative description of the response type

f-brinkmann commented 11 months ago

@isfmiho, @petibub, yes, that seems to work, i.e., it passes the sofar verification. Two follow up questions:

How should be handle the new version? I'm fine with both:

I added some general rules for the new conventions. Is this fine?

f-brinkmann commented 11 months ago

Another question: Would it make sense to rename Data.Emitter and Data.Receiver to Data.Audio or do you prefer the more explicit naming scheme?

isfmiho commented 11 months ago

Another question: Would it make sense to rename Data.Emitter and Data.Receiver to Data.Audio or do you prefer the more explicit naming scheme?

We considered that option too, but with Data.Emitter and Data.Receiver you have the chance to optionally save both data to one file.

isfmiho commented 11 months ago

@isfmiho, @petibub, yes, that seems to work, i.e., it passes the sofar verification. Two follow up questions:

How should be handle the new version? I'm fine with both:

  • Move 0.1 conventions to deprecated folder on sofaconventions.org/conventions and delete them from SOFAtoolbox
  • Don't use v0.2 and apply the changes to v0.1

I suggest to use v0.2 and move v0.1 to deprecated. So we still have a proper link between old versions and old files. If nobody has issues I will update Sofaconventions and copy the files accordingly.

I added some general rules for the new conventions. Is this fine?

  • If GLOBAL:DataType is Audio, the file must have the fields Data.SamplingRate and Data.SamplingRate_Units (value must be hertz)

Data.SamplingRate and Data.SamplingRate_Units are mandatory anyway.

hertz makes sense but do you think we should restrict that? Might there be a case once when a different unit is used?

  • If GLOBAL:SOFAConventions is AnnotatedEmitterAudio the sofa file must contain Data.Emitter
  • If GLOBAL:SOFAConventions is AnnotatedEmitterReceiver the sofa file must contain Data.Receiver

I think you are talking about AnnotatedEmitterAudio and AnnotatedReceiverAudio... -> These are mandatory fields anyway in the corresponding convention. So yes, I agree, please check for these fields. The SOFA Toolbox also checks for mandatory fields.

f-brinkmann commented 11 months ago

I suggest to use v0.2 and move v0.1 to deprecated. So we still have a proper link between old versions and old files. If nobody has issues I will update Sofaconventions and copy the files accordingly.

Sounds good to me, can you ping me once this is done so I can update sofar.

Data.SamplingRate and Data.SamplingRate_Units are mandatory anyway.

In addition to checking it for this convention, this demands that any future Convention of DataType Audio must have these fields. I wasn't completely sure, if we want to restrict that already?

petibub commented 11 months ago

Data.SamplingRate and Data.SamplingRate_Units are mandatory anyway.

In addition to checking it for this convention, this demands that any future Convention of DataType Audio must have these fields. I wasn't completely sure, if we want to restrict that already?

I think that SamplingRate is extremely important - How do you know the sampling rate of Data without it?

f-brinkmann commented 11 months ago

Agree, so let's keep this. @isfmiho - I forgot to repsond that AES69 forces the unit of SamplingRate to always be hertz (although I think it may be upper case as well. But that is checked elsewhere in sofar.)

isfmiho commented 11 months ago

The files are updated on: https://sofacoustics.org/conventions/

'hertz' is the proper case according to the standard, but we also allow capital 'H' in the SOFA Toolbox.

Perfect, I think the rest is clear now, agree?