sofacoustics / SOFAtoolbox

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

Idea and implementation of General convention #40

Closed f-brinkmann closed 2 years ago

f-brinkmann commented 2 years ago

I would suggest to drop the convention, because I do not see a clear way of defining it. This is why:

If the convention is general, the dimensions mrn must be allowed for Data_IR (or Data_Imag). In this case E must defined by another variable. The only chance I see would be EmitterPosition because all other variables referring to the emitter are optional. However the EmitterPosition can also be IC in which case E can not be defined and any toolbox validating the convention or SOFA files of that convention would crash.

Of course the dimension E could be specified by the user. But that would break the general idea of SOFA at least API_MO and sofar.

We would not loose much, if we removed General as long as we have General* with * referring to the data types FIR, FIR-E, TF, TF-E, and SOS (See #39).

petibub commented 2 years ago

You refer to the General.csv from the API, and not the standard, correct? In the standard, I think that it is good to have a pure SOFA convention without any restrictions. In the csv file implementing General convention, we need to provide some DataType and FIR is just an example. When you look to its description, you find a text saying that the datatype can be arbitrary.

f-brinkmann commented 2 years ago

Yes, I was referring to the csv file. I noted that the data type can be arbitrary, however the problem described above exists for all data types. If AES69 defines General, then we need to provide General.csv in order to be able to read and create such SOFA files. Let me know if I am missing something, but I think that General can not be defined by a single General.csv. Consequently, there would have to be multiple General.csv files to cover the different possibilities (because for example E would have to be defined by different variables for different cases - as stated above). I assume that would be hard to implement in the current APIs (that I have a deeper knowledge of) and potentially confusing for the average user to select from multiple General.csv versions when creating such data. I would rather drop it from the Standard and API_MO in favor of General*.csv conventions for each DataType

petibub commented 2 years ago

If the problem of General.csv is the provided DataType as an example, we could remove DataType from the CSV. Then, the user will have to insert a valid datatype.

We use General to create a SOFA file without any restrictions, so it's useful to us. If we drop General from the API, I'd need to create several attributes by hand each time I need a valid SOFA file.

I don't really see the problem: if you know that you're going to use X as datatype, you could just load GeneralX.csv. But if you don't know, General.csv helps. Am I missing something?

And as for the standard: General convention is for users who can't use any of the other conventions, but still consider SOFA to store their acoustic data. In such a case, they can use "General" and do whatever they want within the SOFA rules. Note that the alternative is that they use an other format or they make up a convention name or even worse, they an existing convention outside its parameters.

f-brinkmann commented 2 years ago

Thanks for the patience, I think I start to understand the idea and we might be converging.

1. I am now seeing a use for General.csv. The user wants to store data that is of a new Type, e.g. a 3D tensor field. In that case General.csv makes sense, however, if the user wants to save FIR data, I would already recommend GeneralFIR.csv instead. This would imply that we remove Data_ (DataIR, etc) from General.csv and the user adds whatever Data fit the data they want to save.

2. I'm not sure if dropping DataType and requiring the user to add something reasonable would be a good idea. It seems error prone to me and would also be inconsistent, because DataType is read-only in all other conventions. Instead, we could drop DataType completely and add something similar, e.g., CustomDataType, and CustomDataTypeDescription as mandatory attributes.

Do 1 and 2 make sense to you?

In any case

On last question:

petibub commented 2 years ago

Hi!

Thanks for your thoughts. It's good to see somebody thinking all of this through.

Maybe an important note first: here, we can discuss the implementation issues of the API only - Discussions about the standard should be done with AES69 people. Thus, adding CustomDataType as a mandatory field is clearly a case for the SC of the AES - we could discuss it there.

Going back to the API, dropping DataType is actually not possible because according to AES69, it is mandatory (with FIR as default). Sorry for proposing that in the first place, I was on a wrong way. Thus, in General.csv we'll have to keep DataType and set to FIR as this directly implements the standard.

As far as I can see, the actual issue seems to be that our API crashes when there is no variable using E. Correct? If yes, this is clearly a bug and we need to fix it. As a work-around, in General.csv, we could use EmitterPosition with [E C] as default to prevent that. And make our API more robust ;-).

f-brinkmann commented 2 years ago

True parts of this is for SC. I'll keep thinking anyhow, since we're in the flow:

In AES69, we could introduce 'CUSTOM' as a new DataType in which case we could demand fields such as CustomDataType, and CustomDataTypeDescription to be added. This way General would make more sense to me.

Regarding the API. Yes EmitterPosition could become [e C] but if someone decides to use [I C] we would be left without 'e' again. My understanding of 'General' is that both should be possible, but there might be other ways to look at it.

petibub commented 2 years ago

In AES69, we could introduce 'CUSTOM' as a new DataType ...

I think that any DataType not being standardized would be custom, so in my opinion, there is no need to have a specific keyword 'CUSTOM' for that. An application parses the attribute SOFAConvention first and if it is "General" and the application can read that, the application would read the attribute DataType. If it is one of those the application understands - great!

Also, I just found a restriction: from the standard, Sec. 4.6: "Data represent a distinct behaviour of a particular system and shall be stored as variables with associated attributes following one of the data types defined in Annex C. ". Which is kind of a strong suggestion to use one of the defined datatypes, but does not prohibits to use a made up one. So, for an application, General means: Expect any datatype, but you need to understand at least all those defined in Annex C - maybe even more. Does this make sense?

Regarding the API. Yes EmitterPosition could become [e C] but if someone decides to use [I C] we would be left without 'e' again. My understanding of 'General' is that both should be possible, but there might be other ways to look at it.

Here, I fully agree.

isfmiho commented 2 years ago

Hi, regarding the custom datatypes: The API can be extended with "stable" conventions by creating new .csv files in the conventions subfolder. To test the functionality myself: I made copies of the existing "General_1.0.csv", changing only the name (file name and GLOBAL:SOFAConventions with a text editor), and the data type (text editor).

One file contained the datatype "custom" (it could of course be any other random string as well). The other file contained and empty field "" for its datatype.

Both could be handled by the API, I tested saving and loading them.

I would suggest, if you need different data types (or dimensions), prepare a proper .csv file for that. Commit it to the API if it might be relevant for other users as well!? Once there are data available we can consider implementing it in the standard. This is how it was handled so far.

Do you see any further necessary steps to close this issue?

(We are talking about API issues still. Standard issues should be treated in the standard meetings.)

f-brinkmann commented 2 years ago

No, fine for me