OSOceanAcoustics / echopype

Enabling interoperability and scalability in ocean sonar data analysis
https://echopype.readthedocs.io/
Apache License 2.0
95 stars 73 forks source link

Inconsistency in Platform variable dimensions amongst the sensors #649

Closed b-reyes closed 2 years ago

b-reyes commented 2 years ago

In PR #631 variables were added to the Platform group and if they did not exist, then they were set to NaN.

I believe that a couple of items need to be discussed further.

@leewujung @emiliom @imranmaj what do you think?

emiliom commented 2 years ago
  • The variables transducer_offset_x/y/z, MRU_offset_x/y/z, MRU_rotation_x/y/z, position_offset_x/y/z, and water_level were added to EK60 and EK80. Should we also add these variables to AZFP?

I would think so.

  • In EK60, the variables transducer_offset_x/y/z, MRU_offset_x/y/z, MRU_rotation_x/y/z, and position_offset_x/y/z have the dimension (frequency). However, in EK80 MRU_offset_x/y/z, MRU_rotation_x/y/z, and position_offset_x/y/z do not have a dimension associated with them. For consistency sake, should we make them have the same dimension? Similarly, should we do the same thing for AZFP?

Per the convention, those variables are scalar and should not have any dimensions. So, I would think that they should be scalar for EK60 and AZFP, too.

  • In EK60 water_level has the dimensions (frequency, ping_time). Again, for consistency, should we let EK80 and AZFP have these dimensions too?

Gosh. We had a protracted exchange about water_level (and vertical_offset) in #592. I don't remember the final outcome, but according to the convention water_level is a scalar. The (frequency, ping_time) dimensionality is how it's been in echopype probably for quite a while. I don't know why it's a function of frequency.

See above. As for mru_time in EK80, I think the linkage to #647 is weak; mru_time is being renamed to time2, but that doesn't really inform your question.

leewujung commented 2 years ago
  • The variables transducer_offset_x/y/z, MRU_offset_x/y/z, MRU_rotation_x/y/z, position_offset_x/y/z, and water_level were added to EK60 and EK80. Should we also add these variables to AZFP?

I would think so.

Agree. All three sonar models should have these variables to comply with the convention.

  • In EK60, the variables transducer_offset_x/y/z, MRU_offset_x/y/z, MRU_rotation_x/y/z, and position_offset_x/y/z have the dimension (frequency). However, in EK80 MRU_offset_x/y/z, MRU_rotation_x/y/z, and position_offset_x/y/z do not have a dimension associated with them. For consistency sake, should we make them have the same dimension? Similarly, should we do the same thing for AZFP?

Per the convention, those variables are scalar and should not have any dimensions. So, I would think that they should be scalar for EK60 and AZFP, too.

  • In EK60 water_level has the dimensions (frequency, ping_time). Again, for consistency, should we let EK80 and AZFP have these dimensions too?

Gosh. We had a protracted exchange about water_level (and vertical_offset) in #592. I don't remember the final outcome, but according to the convention water_level is a scalar. The (frequency, ping_time) dimensionality is how it's been in echopype probably for quite a while. I don't know why it's a function of frequency.

The set_group functions tell you where they are from:

  • The dimensions are not consistent from EK60 to EK80 for the variables pitch, roll, and vertical_offset. In EK60 they have dimensions (frequency, ping_time) and in EK80 they have (mru_time). This is partially related to Rename Platform location_time and mru_time #647.

Again the set_group functions tell you where they are from:

@b-reyes : having a better sense of where the data come from is informative in these re-organizations. These will useful in thinking about how to make things potentially more efficient as it is related to how the datagrams are stored in the parser and potential change of parser structure.

b-reyes commented 2 years ago

@leewujung and @emiliom It appears that the following variables need to be added to the Platform group for the AZFP sensor as they are in the convention and their addition would also provide some consistency with EK60/EK80:

leewujung commented 2 years ago

latitude, longitude, pitch, roll

  • Similar to what we have done before, we should set these as NaNs. However, they will need to be NaN arrays as in the convention these variables have a time dimension.

Since AZFP as an instrument would not produce these data, I agree that we should add them as NaN arrays, but perhaps with a time dimension of length 1? (can we have length of 0?)

emiliom commented 2 years ago

In the convention these 4 variables are specified as "MA", Mandatory if applicable or available. So, it seems fine to leave them out if the corresponding data are not available to populate them.

leewujung commented 2 years ago

Good point @emiliom , I did not go back to check the convention. In that case, I agree that we can just leave them out (and users can populate them using externally acquired data via update_platform).

b-reyes commented 2 years ago

In the convention these 4 variables are specified as "MA", Mandatory if applicable or available. So, it seems fine to leave them out if the corresponding data are not available to populate them.

Sounds good. Thank you @emiliom for pointing out that we don't need to add them!

Based on @emiliom and @leewujung's comments, we will not add the variables latitude, longitude, pitch, roll to the AZFP Platform group.

emiliom commented 2 years ago

I think we can close this issue, right?

b-reyes commented 2 years ago

@emiliom yes, I think this can be closed now.