search

sofacoustics / SOFAtoolbox

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

Broken csv Conventions #53

Closed f-brinkmann closed 2 years ago

f-brinkmann commented 2 years ago

The update of the csv-conventions contains errors. Here are the questions and issues in several cases the changes violate AES69:

GeneralFreeFieldDirectivity 1.0:

GeneralFreeFieldDirectivity 1.1:

FreeFieldHRTF 1.0:

@petibub @isfmiho can we discuss future convention updates before pushing to master? sofar also uses the conventions files and pushing changes without any testing can break SOFAtoolbox and sofar.

petibub commented 2 years ago

The update of the csv-conventions contains errors. Here are the questions and issues in several cases the changes violate AES69:

GeneralFreeFieldDirectivity 1.0:

  • Data.Real: Dimensions must be mrn not MRN. Otherwise these dimensions are not defined.

Agreed.

GeneralFreeFieldDirectivity 1.1:

  • EmitterPosition: Dimensoins must be 'eC, eCM' not 'EC, ECM'. Otherwise these dimensions are not defined.

Agreed (see Tab. D.15.4. and D.15.5F)

Why was this field depend on E?

I don't understand this.

The EmitterDescription has Dimensions 'IS, MS' This seems to be inconsistent

Standard says 'MS' because than you can change the emitter description each time you do a new measurement M. Would you like to have it depended on E as well? We still can add it to the standard, having 'MS' and 'ES'.

  • GLOBAL:EmitterDescriptions: I think so far we only have GLOBAL attributes but not GLOBAL variables. Do we want this? Is this intended by AES69 (Only GLOBAL attributes are mentiond so far)? Are global variables possible in NETCDF (Not sure, but I don't think so)? Why do we need it at all, we have already have the M-dependent EmitterDescription?

All objects are global. That's a typo in the CSV, it should be just EmitterDescriptions.

FreeFieldHRTF 1.0:

  • Data.Real must have Dimensions mrne instead of MRNE, otherwise these dimensions are not defined.

Yep, agreed.

@petibub @isfmiho can we discuss future convention updates before pushing to master? sofar also uses the conventions files and pushing changes without any testing can break SOFAtoolbox and sofar.

Good idea. We're currently preparing the SOFA toolbox for the releases, so we push changes quickly - sorry for breaking stuff. This should be over in the next few days, when having the release.

@isfmiho: could you fix the CSV when back at work? Thanks a lot!

Regards, Piotr

f-brinkmann commented 2 years ago

Thanks for the quick feedback. I discussed the EmitterDescription with @david-ackermann and we would suggest the following

Everything else would be solved by @petibub suggestions.

Exited for the release. Can you notify me right before? I can test the conventions with sofar in that case. May I also suggest to use the release to switch to the more common branch convention of having the release on master and current work on develop?

f-brinkmann commented 2 years ago

ps. How about using the new release to switch to sharing the same SOFA conventions from https://github.com/pyfar/sofar_conventions by including it as a git submodule? I would also be fine with moving the repository to https://github.com/sofacoustics.

petibub commented 2 years ago

Thanks for the quick feedback. I discussed the EmitterDescription with @david-ackermann and we would suggest the following

  • Have GLOBAL:EmitterDescription as an optional attribute (not string variable)

Perfect - that's how it is in the standard.

  • Have EmitterDescriptions as an optiona string variable of dimension IS, MS, ES

I agree for 'MS' and 'ES'. However, 'IS' would be just a copy of the attribute EmitterDescription, thus, it does not make much sense. As for 'MS' and 'ES', we have a similar situation for SingleRoomSRIR, in which we write that EmitterDescriptions can depend on [M S], [E S], or [M E S]. The corresponding text is:

If the individual emitters require individual descriptions, EmitterDescriptions shall change for each E. If the IRs are re-measured and the source change, EmitterDescriptions shall change for each M. If, in that case, the individual emitters require individual descriptions, EmitterDescriptions shall change for each M and E.

Would that also work for FreeFieldDirectivity?

Everything else would be solved by @petibub suggestions.

Exited for the release. Can you notify me right before? I can test the conventions with sofar in that case.

Yep, we'll do!

May I also suggest to use the release to switch to the more common branch convention of having the release on master and current work on develop?

We're a too small team for having a complex branching system. Our releases are intended to be stable, development is on github code repository. We have discussed the issue of machine-readable convention file previously in the standard committee and decided that the stable conventions files go to https://www.sofaconventions.org/conventions/. As soon as we release SOFA 2.0 CSV file, you'll find them files there.

petibub commented 2 years ago

ps. How about using the new release to switch to sharing the same SOFA conventions from https://github.com/pyfar/sofar_conventions by including it as a git submodule? I would also be fine with moving the repository to https://github.com/sofacoustics.

Do you refer to the JSON files - We rely on CSV... Or do you mean your copy of our CSV files - Not sure what that copy is... In any case, have discussed that issue previously in the standard committee and the stable conventions files go to https://www.sofaconventions.org/conventions/. As soon as we release SOFA 2.0 CSV file, you'll find them files there.

As for moving sofar to sofacoustics: no problems with that. Tell me if you wanna do that, I'll switch on corresponding rights for you...

f-brinkmann commented 2 years ago

I agree for 'MS' and 'ES'. However, 'IS' would be just a copy of the attribute EmitterDescription, thus, it does not make much sense. As for 'MS' and 'ES', we have a similar situation for SingleRoomSRIR, in which we write that EmitterDescriptions can depend on [M S], [E S], or [M E S]. The corresponding text is:

[M E S] is also fine - we were also thinking of it but did not suggest it because we did not see a use case. But it does not harm to have it there

We're a too small team for having a complex branching system. Our releases are intended to be stable, development is on github code repository. We have discussed the issue of machine-readable convention file previously in the standard committee and decided that the stable conventions files go to https://www.sofaconventions.org/conventions/. As soon as we release SOFA 2.0 CSV file, you'll find them files there.

It would still be simple, only develop and master. The problem I see is that the master usually contains a stable version of the code, which is not the case now.

Do you refer to the JSON files - We rely on CSV... Or do you mean your copy of our CSV files - Not sure what that copy is... In any case, have discussed that issue previously in the standard committee and the stable conventions files go to https://www.sofaconventions.org/conventions/. As soon as we release SOFA 2.0 CSV file, you'll find them files there.

Thats great. Would it be ok, to also store the conventions in json format and have a converter for csv to json in the repository? We could have the folders csv and json and a convert_csv_to_json.py. This way I could close my conventions repository and use the official one as a submodule. I know I'm sometimes annoyingly persinsting on things, but I would suggest the master / develop structure there as well. In case the conventions repository gets used by others the master should always be stable.

petibub commented 2 years ago

[M E S] is also fine - we were also thinking of it but did not suggest it because we did not see a use case. But it does not harm to have it there

OK, let's go for [MS], [ES], and [MES] - which is consistent with some other conventions.

We're a too small team for having a complex branching system. Our releases are intended to be stable, development is on github code repository. We have discussed the issue of machine-readable convention file previously in the standard committee and decided that the stable conventions files go to https://www.sofaconventions.org/conventions/. As soon as we release SOFA 2.0 CSV file, you'll find them files there.

It would still be simple, only develop and master. The problem I see is that the master usually contains a stable version of the code, which is not the case now.

So, you use "master" for releases, i.e., master is only updated when releasing a new version? This makes releases obsolete, doesn't? And what about tagging - we tag the repository at the commit used for the release; would that be obsolete as well? Hmmm...

Do you refer to the JSON files - We rely on CSV... Or do you mean your copy of our CSV files - Not sure what that copy is... In any case, have discussed that issue previously in the standard committee and the stable conventions files go to https://www.sofaconventions.org/conventions/. As soon as we release SOFA 2.0 CSV file, you'll find them files there.

Thats great. Would it be ok, to also store the conventions in json format and have a converter for csv to json in the repository? We could have the folders csv and json and a convert_csv_to_json.py. This way I could close my conventions repository and use the official one as a submodule.

I like this idea - we actually do not need special structure, we could store .json and .csv files in the same directory. And if you have an automatic converter in your toolbox, that's cool, that' how we can stay consistent between these two types of representations.

I know I'm sometimes annoyingly persinsting on things, but I would suggest the master / develop structure there as well. In case the conventions repository gets used by others the master should always be stable.

I'm afraid that I don't understand that. Do you propose to have a separate repository for convention files? (one for SOFA Toolbox, one for sofar, and a third one for conventions)? Sorry, if I'm missing something very basic here...

f-brinkmann commented 2 years ago

So, you use "master" for releases, i.e., master is only updated when releasing a new version? This makes releases obsolete, doesn't? And what about tagging - we tag the repository at the commit used for the release; would that be obsolete as well? Hmmm...

You can still Tag and release (we also do this in addition in pyfar). I think the biggest point might be that anyone who clones the repository without giving it much thought will always get a stable version because the master is usually the default branch.

I'm afraid that I don't understand that. Do you propose to have a separate repository for convention files? (one for SOFA Toolbox, one for sofar, and a third one for conventions)? Sorry, if I'm missing something very basic here...

Sorry, I think I had a Freudscher Verleser :) Personally, I think a repository for the conventions that can be used by all toolboxes that want to, would be the best solution - but I'm fine if we do not do this. I see a slight danger of diverging files, if we manually maintain a copy on sofaconventions.org.

petibub commented 2 years ago

You can still Tag and release (we also do this in addition in pyfar). I think the biggest point might be that anyone who clones the repository without giving it much thought will always get a stable version because the master is usually the default branch.

OK. So, development only in a "development" branch, master branch updated only when releasing. Development branch tagged on the release. A release is a zipped but otherwise the exact copy of each master branch commit. Correct?

Sorry, I think I had a Freudscher Verleser :) Personally, I think a repository for the conventions that can be used by all toolboxes that want to, would be the best solution - but I'm fine if we do not do this. I see a slight danger of diverging files, if we manually maintain a copy on sofaconventions.org.

My plan is to copy on release. This would be one step within the release process, thus, not really prone to errors. OK?

f-brinkmann commented 2 years ago

OK. So, development only in a "development" branch, master branch updated only when releasing. Development branch tagged on the release. A release is a zipped but otherwise the exact copy of each master branch commit. Correct?

exactly :)

My plan is to copy on release. This would be one step within the release process, thus, not really prone to errors. OK?

Yes, fine for me. I would keep my copy at https://github.com/pyfar/sofar_conventions in that case and also update it upon releases

petibub commented 2 years ago

Just realized that if we switch the development to "development" branch and reset master to the last release, all conventions files will be reset to SOFA 1.0 on master. @f-brinkmann You sure you want it?

f-brinkmann commented 2 years ago

No, lets wait until the next release and move to develop after that. Would that be fine?

petibub commented 2 years ago

OK.