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

Add beam dimension to Beam group #520

Closed emiliom closed 2 years ago

emiliom commented 2 years ago

(Updated 2022-3-30). Add a beam dimension to Beam_groupX groups. We've decided to always include the beam dimension, instead of relying on an implicit length-1 dimension.

Older notes (just for reference):

emiliom commented 2 years ago

Note about a related change in the draft SONAR-netCDF4 version 2: Under 7.1. Significant changes from version 1 to version 2:

Beam: Added variables for the description and storage of data from split-aperture beams via a subbeam dimension added in the Sonar group.

emiliom commented 2 years ago

One thing I haven't looked into is what value the single-element (length 1) beam coordinate variable should have. I'd be curious to see what kinds of values are currently assigned to the EK80 beam (formerly quadrant) coordinate. What do they represent?

emiliom commented 2 years ago

Here is the definition of the beam coordinate in the convention, including its data type:

string beam(beam):  Beam name (or number or identification code).
:long_name = "Beam name"
b-reyes commented 2 years ago

To tackle the first item in this issue, we need to address a few items:

emiliom commented 2 years ago

Based on a comment by @emiliom, it appears that the convention has beam as a string. For EK80 in echopype, quadrant is currently an integer

Ah. I think it should be changed to string type, so that a string type beam coordinate is used everywhere.

After talking with @emiliom, it seems like when adding the coordinate beam to the other sensors (such as EK60), a rule to follow is to add the beam dimension to all variables that have (frequency, ping_time, range_sample). Does this seem fair @leewujung?

It just occurred to me to look this up in the convention. It's not a perfect guide, because the dimensionality of Beam_groupX variables like backscatter_i are the biggest, deliberate structural modification we make to the convention in echopype. But with that caveat, the convention lists beam as the last dimension. That's consistent with what's used with EK80.

And finally, just a reminder that quadrant no longer exists in dev. It's been changed to beam.

leewujung commented 2 years ago

Based on a comment by @emiliom, it appears that the convention has beam as a string. For EK80 in echopype, quadrant is currently an integer

Ah. I think it should be changed to string type, so that a string type beam coordinate is used everywhere.

In the EK80 case I think it would still be 0, 1, 2, 3 (or 1, 2, 3, 4) since here we are setting each quadrant (or sector) of a transducer to a "beam" -- note that how people define a beam can vary depending on the context -- whether or not it corresponds to an actual physical elements, or is it a beamformed results, etc.

A potential problem is that when setting the beam to strictly of string type is that users can get confused trying to index using integer instead of string (i.e., .sel(beam=1) instead of .sel(beam="1")). Though string type is more flexible for sure, especially when the beam has an actual name or an actual reference ID that is not a number.

the convention lists beam as the last dimension. That's consistent with what's used with EK80.

I think it is also the most convenient during computation, since many of the operations would end up doing something like .mean(dim="beam"). There is no difference at the syntax level, but it just feels cleaner, especially if we were to squeeze out the dim=1 beam dimension for single beam echosounder data. I wonder if this may actually have impact computationally later re the chunking business.

b-reyes commented 2 years ago

@leewujung and @emiliom thank you for your input. Here is my summary so far:

Items that still need to be addressed:

b-reyes commented 2 years ago

I am currently reviewing an EK80 sensor with the current dev version and I see for this sensor we added the data variables: beam_group_descr, beam_group_name, frequency, serial_number, sonar_model, sonar_serial_number, sonar_software_name, sonar_software_version to the Sonar group.

emiliom commented 2 years ago

Regarding the data type for beam: Unless there are truly compelling reasons not to follow the convention, I think we should follow it and use string type. I agree with @leewujung that users can get confused when the beam value is numeric, but that could also be said of any similar situation. Users should be aware of what data types they're dealing with :wink:.

Regarding frequency, serial_number, sonar_model, sonar_serial_number, sonar_software_name, sonar_software_version in the Sonar group: The convention states that those are global attributes, not variables. I assume they were turned into variables with a frequency dimension for a good reason that's specific to EK80. If so, we're in a pickle. echopype is breaking the convention probably b/c the v1 convention doesn't support these type of sensors and usage. I'm hesitant to suggest that we break the convention for all other sensors without first having a dedicated discussion. More broadly, this isn't an issue that's directly related to the addition of the beam dimension, right?

emiliom commented 2 years ago

For example, we could let beam be an integer. Then add beam_name or beam_id as a variable to the Sonar group. Using this, a person could know the name/ID by doing ed['Sonar'].beam_name.isel(beam=0) .

Actually, since a string type beam coordinate would contain the beam id/label, an additional variable seems unnecessary. ed['Sonar/Beam_group1'].beam.isel(beam=0) will do what you're saying (note that I changed the group path to point to "Sonar/Beam_group1"

b-reyes commented 2 years ago

Regarding the data type for beam: Unless there are truly compelling reasons not to follow the convention, I think we should follow it and use string type. I agree with @leewujung that users can get confused when the beam value is numeric, but that could also be said of any similar situation. Users should be aware of what data types they're dealing with 😉.

It makes sense to use a string type for the beam dimension (especially because that is the convention specification). I guess the question is, what values will we be assigning to beam? I have been doing some digging into the parsed datagrams and I can't find a name/ID associated with the beam for EK80. @emiliom or @leewujung can you tell me what variable in the datagram would correspond to the name/ID?

Regarding frequency, serial_number, sonar_model, sonar_serial_number, sonar_software_name, sonar_software_version in the Sonar group: The convention states that those are global attributes, not variables. I assume they were turned into variables with a frequency dimension for a good reason that's specific to EK80. If so, we're in a pickle. echopype is breaking the convention probably b/c the v1 convention doesn't support these type of sensors and usage. I'm hesitant to suggest that we break the convention for all other sensors without first having a dedicated discussion. More broadly, this isn't an issue that's directly related to the addition of the beam dimension, right?

Since there seems to be items beyond the addition of the beam dimension, I suggest we create a new issue for this, if you think this is a problem. @emiliom thank you for your input here.

I found that for EK80 there is no beam dimension in Beam_group2, should this be added to the issue @emiliom and @leewujung?

leewujung commented 2 years ago

can you tell me what variable in the datagram would correspond to the name/ID?

For beam name or beam ID, I don't think there's that in EK80 files.

I found that for EK80 there is no beam dimension in Beam_group2, should this be added to the issue

For Beam_group2 because it is power/angle format, data from the "beams" are processed to extract the angle data, so there is no "beam" per se. BUT, to make it consistent, I think we will be adding a beam dimension with length 1. @emiliom please confirm.

Since there seems to be items beyond the addition of the beam dimension, I suggest we create a new issue for this, if you think this is a problem.

I agree with creating another issue for this -- I haven't had a chance to think through and dig through what the variations may be across different sonar models but it'll be nice to have a focused discussion there.

b-reyes commented 2 years ago

For beam name or beam ID, I don't think there's that in EK80 files.

Since there is no name or beam ID for EK80 and this seems to not exist for EK60 too, it looks like we are left with using numbers. If this is correct, I suggest that for 4 beams we let beam = ["0", "1", "2", "3"] (so that we follow the convention). In short, the only real change we are making is setting beam as a string, rather than an int for EK80 Beam_group1. Additionally, for EK60 and AZFP, we would have beam = ["0"].

emiliom commented 2 years ago

can you tell me what variable in the datagram would correspond to the name/ID?

I agree with what's been stated in the preceding comments. The only tweak I'd make is to use base 1 for the string label. So, for the length-1 beam coordinate, the value would be "1". That seems a little more user friendly than "0". The convention doesn't say anything about a preference for numbers vs labels/ids, let alone for base 0 vs base 1 for numbers as beam values.

I found that for EK80 there is no beam dimension in Beam_group2, should this be added to the issue

For Beam_group2 because it is power/angle format, data from the "beams" are processed to extract the angle data, so there is no "beam" per se. BUT, to make it consistent, I think we will be adding a beam dimension with length 1. @emiliom please confirm.

Indeed, @b-reyes and I met yesterday and realized that the EK80 Beam_group2 has no beam dimension. I definitely recommend that we be completely consistent and add the length-1 beam dimension, just like we'll be doing with EK60 and AZFP Beam_group1.

Since there seems to be items beyond the addition of the beam dimension, I suggest we create a new issue for this, if you think this is a problem.

I agree with creating another issue for this -- I haven't had a chance to think through and dig through what the variations may be across different sonar models but it'll be nice to have a focused discussion there.

:+1:

leewujung commented 2 years ago

I agree with @emiliom's comment above!

b-reyes commented 2 years ago

@emiliom and @leewujung thank you! It looks like we have arrived at a consensus. Just to summarize the changes that will take place:

b-reyes commented 2 years ago

The following is a discussion on what variables should have the beam dimension (for EK60). Whether or not to include the beam dimension was initially based on the 1.0 convention.

Based on EK60

@leewujung has made the following comment on the variables beam_direction_x/y/z, angle_offset_alongship/athwartship, and angle_sensitivity_alongship/athwartship for EK80. This is regarding the addition of the beam dimension to the variables.

"there is only 1 transducer and 4 "beams" included in EK80. If we were to add the beam dimension, maybe duplicate all values for all 4 beams"

Currently, in the convention equivalent_beam_angle and gain_correction have the dimensions (ping_time, beam). Right now echopype has the dimension as only (frequency). @leewujung made the following comments on this subject:

"These variables are frequency dependent so the frequency (renamed to channel) dimension has to be retained. For EK60 this does not change across ping_time. If convention requires ping_time the value should be duplicated."

leewujung commented 2 years ago

Thanks @b-reyes.

For beam_direction_x/y/z, angle_offset_alongship/athwartship, and angle_sensitivity_alongship/athwartship: I think more context should be added here for making a decision.

The 4 "beams" stored in EK80 are different elements of a split-beam transducer. The number of elements could also be 3. I think adding a length=1 beam dimension to these variables can be confusing to users, because these numbers are meant to be applied as part of the split-beam angle processing, which utilize phase info in the waveform (complex data) from all 4 elements.

Many of the same split-beam transducers can be used interchangeably between EK60 and EK80. The reason why you don't see data from these 4 elements is because EK60 does this processing and store only the split-beam angle (along with power) in the data.

In other words, users do not have access to the "raw" complex data in EK60, but they do for EK80.

Users can select whether they want EK80 to store the "raw" complex data or the "processed" power/angle data in a form comparable to EK60, separately for each channel. That is why you can have co-existing complex and power/angle data in a single EK80 file.

leewujung commented 2 years ago

@b-reyes : based on our discussions, below is what I think at the moment for the dimensions. Let me know if some of these do not work with your understanding of the convention.

Here we are taking an approach of "addition" to only add dimensions to the variables if they do not already exist, but we DO NOT remove any dimensions from the convention. So, in practice, this means that we have a lot of identical values along the dimensions that are defined in the convention but are single values from the data file.

As @b-reyes suggested, let's take EK60 first:

EK60: Sonar/Beam_group1

Update 2022/04/14 8:39 PM: I realized I made a mistake for the last bullet point. For these echopype-added variables, let's NOT add the beam dimension as @b-reyes suggested.

b-reyes commented 2 years ago

@leewujung thank you for putting that list together. Here are also some other notes for EK60 (some items have a strikethrough them, please ignore them and see my comment below)

emiliom commented 2 years ago

Thanks @leewujung and @b-reyes for compiling these!

@b-reyes , one general comment: You list a few variables that don't have a frequency dimension in the convention but do in echopype but are otherwise identical (transmit_bandwidth, transmit_duration_nominal, transmit_power transducer_offset_x/y/z). This is the deliberate result of one of the core changes that echopype introduced relative to the convention. The frequency dimension (soon to be channel) in Beam_groupX groups is our addition resulting from our restructuring of the Beam_groupX data. As @leewujung stated in #566: "Currently we use frequency as a dimension in the echopype-generated dataset (in place of having data from different frequency channels saved into different Beam_groupXs defined in the SONAR-netCDF4 convention ver. 1.0)."

So, all such cases can be treated as following the convention, so to speak.

b-reyes commented 2 years ago

@b-reyes , one general comment: You list a few variables that don't have a frequency dimension in the convention but do in echopype but are otherwise identical (transmit_bandwidth, transmit_duration_nominal, transmit_power transducer_offset_x/y/z). This is the deliberate result of one of the core changes that echopype introduced relative to the convention.

@emiliom thank you for pointing that out. I thought that might be the case, but I wanted to make sure. Taking this into account, does the following look more correct for EK60?

emiliom commented 2 years ago

To adhere to the convention, we should let beam_type have dims (channel, ping_time)

As you listed earlier, for beam_type echopype uses only a frequency dimension while the convention states a ping_time dimension. By our own logic and what the convention says, it should have (channel, ping_time) dimensions. Personally, I find it odd that the convention should define it as being a function of time. @leewujung could you chime in?

Current echopype variables that do not need their dimensions modified

Correct

leewujung commented 2 years ago

To adhere to the convention, we should let beam_type have dims (channel, ping_time)

As you listed earlier, for beam_type echopype uses only a frequency dimension while the convention states a ping_time dimension. By our own logic and what the convention says, it should have (channel, ping_time) dimensions. Personally, I find it odd that the convention should define it as being a function of time. @leewujung could you chime in?

It is indeed odd that beam_type can change along ping_time. The convention p.14 lists beam_type as single or split beam. For EK60, from the data spec it seems possible to config the system to only have power without angle data (have split beam functioning as single beam), but I am not sure if that is something always hardwired to the type of transducers or configurable from the software.

I don't know if this is something "fishery sonar" systems can do easily by configuring beamforming through software. @gavinmacaulay could you help us our here?

leewujung commented 2 years ago
  • Do not appear in the convention. Should we modify these?
    • channel_id
      • EP - (frequency)
    • gpt_software_version
      • EP - (frequency)
    • offset
      • EP - (frequency, ping_time)
leewujung commented 2 years ago

angle_offset_alongship/athwartship and angle_sensitivity_alongship/athwartship are not currently in the convention. So, we have some wiggle room here. Maybe they can have dim (channel)?

Since we are changing the dimensions of all the other similar variables from the raw dimension (channel) to be more compliant with the convention (channel, ping_time, beam), I kinda think we should keep this variable consistent with the other variables. @emiliom : do you have a recommendation here?

leewujung commented 2 years ago

@b-reyes : Please note that I made an edit at the bottom of https://github.com/OSOceanAcoustics/echopype/issues/520#issuecomment-1099455095 because I realized a mistake when I originally wrote it.

leewujung commented 2 years ago

As for how the alongship/athwartship maps to the beamwidth_receive/transmit_major/minor in the convention, I am not sure.

The convention wants the 1-way beamwidths (p.16), but I think what EK60 data contains are 2-way beamwidths (transmit-receive characterized together) (also noted in TODO in the code). I found it interesting that beamwidth_receive_major/minor are listed as "M" and beamwidth_transmit_major/minor are listed as "MA" (mandatory when applicable). For the type of transducers used in EK60, the transmit and receive should be reciprocal, but I would hesitate in splitting one value into two and store them separately.

For the major vs minor issue... I am not entirely sure how to apply the description on p.25. I'd map the "horizontal" direction to the athwartship (y) direction, and the "vertical" direction to the alongship (x) direction, but I don't intuitively understand what horizontal and vertical mean for a downward looking transducer.

@emiliom : what do you suggest we do for these two questions?

gavinmacaulay commented 2 years ago

It is indeed odd that beam_type can change along ping_time. The convention p.14 lists beam_type as single or split beam. For EK60, from the data spec it seems possible to config the system to only have power without angle data (have split beam functioning as single beam), but I am not sure if that is something always hardwired to the type of transducers or configurable from the software.

I don't know if this is something "fishery sonar" systems can do easily by configuring beamforming through software.

I don't remember why beam_type is specified with a ping_time axis. It does seem somewhat unnecessary, but maybe I was thinking of/anticipating the Simrad SN90 sonar which in theory has the potential to change the beam type of the inspection beams at will.

gavinmacaulay commented 2 years ago

As for how the alongship/athwartship maps to the beamwidth_receive/transmit_major/minor in the convention, I am not sure.

The convention wants the 1-way beamwidths (p.16), but I think what EK60 data contains are 2-way beamwidths (transmit-receive characterized together) (also noted in TODO in the code). I found it interesting that beamwidth_receive_major/minor are listed as "M" and beamwidth_transmit_major/minor are listed as "MA" (mandatory when applicable). For the type of transducers used in EK60, the transmit and receive should be reciprocal, but I would hesitate in splitting one value into two and store them separately.

For the major vs minor issue... I am not entirely sure how to apply the description on p.25. I'd map the "horizontal" direction to the athwartship (y) direction, and the "vertical" direction to the alongship (x) direction, but I don't intuitively understand what horizontal and vertical mean for a downward looking transducer.

The convention specified 1-way beamwidths because they can change from ping to ping in an omni sonar, especially the receive beams. For un-beamformed beams using the same transducer for transmit and receive the intention in the convention was that the 2-way value would be halved and stored in the transmit and receive beamwidth variables.

In section 4.2 (page 25) of the convention, having the y-axis horizontal means that platform roll has been compensated for. The association of major with horizontal and minor with vertical is in the context of omni sonars beam that point sort of horizontally (so major axis is left/right and minor axis up/down). For a downwards-looking echosounder beam on a vessel, major beam angles are athwartship and minor are alongship (this is made clearer in v2 of the convention, in which echosounders are explicitly supported).

leewujung commented 2 years ago

@gavinmacaulay thanks for the clarifications!

The convention specified 1-way beamwidths because they can change from ping to ping in an omni sonar, especially the receive beams. For un-beamformed beams using the same transducer for transmit and receive the intention in the convention was that the 2-way value would be halved and stored in the transmit and receive beamwidth variables.

Hmm, ok. @emiliom: let us know what you think about just splitting the 2-way beamwidth into transmit and receive and store separately.

In section 4.2 (page 25) of the convention, having the y-axis horizontal means that platform roll has been compensated for. The association of major with horizontal and minor with vertical is in the context of omni sonars beam that point sort of horizontally (so major axis is left/right and minor axis up/down). For a downwards-looking echosounder beam on a vessel, major beam angles are athwartship and minor are alongship (this is made clearer in v2 of the convention, in which echosounders are explicitly supported).

Great, then we have something to follow here.

@b-reyes: For these definitions, once the 2-way beamwidth issue is settled, please put in a note in the comments in the set groups methods. Thanks!

emiliom commented 2 years ago

angle_offset_alongship/athwartship and angle_sensitivity_alongship/athwartship are not currently in the convention. So, we have some wiggle room here. Maybe they can have dim (channel)?

Since we are changing the dimensions of all the other similar variables from the raw dimension (channel) to be more compliant with the convention (channel, ping_time, beam), I kinda think we should keep this variable consistent with the other variables. @emiliom : do you have a recommendation here?

(This may get confusing since I'm responding to comments some way up). First, I hadn't noticed that in the convention all Beam_groupX variables are a function of ping_time, while in echopype that dimentionality had been dropped in a number of variables. Many of the variables @leewujung listed under "Variables having dims (channel, ping_time, beam): these are vectors (dim: channel) in the data file and in reality does not vary across ping_time" were in this category. It does seem like the alongship/athwartship variable introduced in echopype ought to have a beam dimension, based on their definition being analogous to the major/minor variables.

emiliom commented 2 years ago

In section 4.2 (page 25) of the convention, having the y-axis horizontal means that platform roll has been compensated for. The association of major with horizontal and minor with vertical is in the context of omni sonars beam that point sort of horizontally (so major axis is left/right and minor axis up/down). For a downwards-looking echosounder beam on a vessel, major beam angles are athwartship and minor are alongship (this is made clearer in v2 of the convention, in which echosounders are explicitly supported).

Hmm. On the one hand @leewujung 's tentative interpretation of athwartship -> major and alongship -> minor is consistent with what @gavinmacaulay is saying. But in that interpretation, the definitions of the minor variables would actually have to change relative to those found in the v1 convention. That seems messy: reuse the existing variable names but change their meanings. I don't see an unambiguous solution. @leewujung for an echosounder on a mooring, do athwarship/alongship suffixes make sense? Despite the generality of major/minor suffixes, are they more appropriate for such cases, if a proper definition (long_name) is used?

emiliom commented 2 years ago

The convention specified 1-way beamwidths because they can change from ping to ping in an omni sonar, especially the receive beams. For un-beamformed beams using the same transducer for transmit and receive the intention in the convention was that the 2-way value would be halved and stored in the transmit and receive beamwidth variables.

Hmm, ok. @emiliom: let us know what you think about just splitting the 2-way beamwidth into transmit and receive and store separately.

I think I'm out of my depth here. I don't think I understand what 1-way or 2-way beamwidth mean.

leewujung commented 2 years ago

For the athwartship/alongship vs major/minor question:

Hmm. On the one hand @leewujung 's tentative interpretation of athwartship -> major and alongship -> minor is consistent with what @gavinmacaulay is saying. But in that interpretation, the definitions of the minor variables would actually have to change relative to those found in the v1 convention. That seems messy: reuse the existing variable names but change their meanings. I don't see an unambiguous solution.

Thinking out loud here, it seems that we can do the following:

  1. use what @gavinmacaulay pointed out in v2 in our compliance in v1 and describe clearly in the comments attribute this change and reference the sources.
  2. set *_major/minor to NaN and use *_athwartship/alongship in the variables as in the raw data, and describe clearly that this is a decision we've made in echopype.

I am inclined slightly to option 2 so that there is no ambiguity in interpreting the variables on what they are. When we need to go to v2, connecting the right variables to according the v2 definitions is straightforward.

for an echosounder on a mooring, do athwarship/alongship suffixes make sense? Despite the generality of major/minor suffixes, are they more appropriate for such cases, if a proper definition (long_name) is used?

I am not sure, I think they are equivalent. In the case of a mooring, the echosounder can be installed with the athwartship/alongship orientation (there's an arrow on the back of the transducer to indicate the "front") aligned to some North-South/East-West directions, the fish tracks extracted from the echoes then are meaningful. (Though of course depending on the mooring mechanism things can get pushed around.) So, I don't see a clear winner in this context.

leewujung commented 2 years ago

For the 1-way or 2-way beamwidth question:

@emiliom : No worries! 1-way just means that you are going from one point to another (transmitter to scatterer, or scatterer to receiver), and 2-way is going from one point to another and back (transmitter to scatterer and back to receiver). When the same transducer is used as both the transmitter and receiver, like in split-beam echosounders, the 2-way beamwidth can be derived from 1-way beamwidth. Let's say the 1-way beampattern function is B(polar angle), the 2-way beampattern function is B^2.

And just noticed this when re-reading the comments above:

For un-beamformed beams using the same transducer for transmit and receive the intention in the convention was that the 2-way value would be halved and stored in the transmit and receive beamwidth variables.

@gavinmacaulay -- wouldn't it be multiplied by 2 to go from 2-way to 1-way beamwidth since the beam is narrower when the same beampattern is applied twice?

emiliom commented 2 years ago
  1. set _major/minor to NaN and use _athwartship/alongship in the variables as in the raw data, and describe clearly that this is a decision we've made in echopype.

I am inclined slightly to option 2 so that there is no ambiguity in interpreting the variables on what they are. When we need to go to v2, connecting the right variables to according the v2 definitions is straightforward.

I agree. Plus, the suggestion you made in 1 to use a comment attribute (BTW, it should be singular, not plural) can be extended to the athwartship/alongship variables to indicate their relationship to the major/minor variables.

Regarding the applicability of athwartship/alongship to moored deployments, my point was that those terms don't seem broadly intuitive or obvious in a mooring platform context -- unless they are actually used in moorings :confused:. But given the motivations above for retaining athwartship/alongship, this potential ambiguity can be set aside.

b-reyes commented 2 years ago
  1. set _major/minor to NaN and use _athwartship/alongship in the variables as in the raw data, and describe clearly that this is a decision we've made in echopype.

@leewujung and @emiliom I am slightly confused here. Are we going with option 1 and 2? For EK60 we have the following values: angle_offset_alongship/athwartship, angle_sensitivity_alongship/athwartship, beamwidth_receive_alongship/athwartship, beamwidth_transmit_alongship/athwartship, and angle_alongship/athwartship. Based on option 2, it looks like we should keep all of these values as is and create the new variables angle_offset_major/minor, angle_sensitivity_major/minor, beamwidth_receive_major/minor, beamwidth_transmit_major/minor, and angle_major/minor filled with NaNs. It seems like this is all based on something that will be changed in v2. If I am understanding this correctly, then I think we should only go with option 1 (the comment attribute on all _alongship/athwartship variables) because v0.6.0 of echopype is purely for v1 of the convention.

emiliom commented 2 years ago

re: 1-way vs 2-way beamwidth:

So, per TODO in the code, it looks like the 2-way beamwidths from the raw datagrams (beam_params["beamwidth_alongship"] and beam_params["beamwidth_athwartship"]) are being assigned to the what are defined as 1-way beamwidths, right? If so, that looks like it should be changed, plus if I followed the discussion correctly it looks like it's an easy conversion.

But we're diverging from the core of this issue. Same goes for the decision about athwartship/alongship vs major/minor. A new issue should be opened, IMHO

gavinmacaulay commented 2 years ago

Replies to some of the comments/questions above:

for an echosounder on a mooring, do athwarship/alongship suffixes make sense?

Not really - the athwartship/alongship became common because ships were the main use of Simrad gear for many years. The use of sounders on other platforms is where the use of major/minor came in as a more general nomenclature. For mooring deployments, the tack is to define the direction of the mooring minor/major axis and then apply a platform heading to get true pointing direction (just as for a vessel).

@gavinmacaulay -- wouldn't it be multiplied by 2 to go from 2-way to 1-way beamwidth since the beam is narrower when the same beampattern is applied twice?

Indeed, yes! I didn't think that one through fully.

b-reyes commented 2 years ago

re: 1-way vs 2-way beamwidth:

So, per TODO in the code, it looks like the 2-way beamwidths from the raw datagrams (beam_params["beamwidth_alongship"] and beam_params["beamwidth_athwartship"]) are being assigned to the what are defined as 1-way beamwidths, right? If so, that looks like it should be changed, plus if I followed the discussion correctly it looks like it's an easy conversion.

But we're diverging from the core of this issue. Same goes for the decision about athwartship/alongship vs major/minor. A new issue should be opened, IMHO

@emiliom I agree with your statement, this seems like it should be moved to a different issue. Is this topic something that needs to be completed before v0.6.0 can be released?

leewujung commented 2 years ago

re: 1-way vs 2-way beamwidth:

So, per TODO in the code, it looks like the 2-way beamwidths from the raw datagrams (beam_params["beamwidth_alongship"] and beam_params["beamwidth_athwartship"]) are being assigned to the what are defined as 1-way beamwidths, right? If so, that looks like it should be changed, plus if I followed the discussion correctly it looks like it's an easy conversion.

@emiliom I agree with your statement, this seems like it should be moved to a different issue. Is this topic something that needs to be completed before v0.6.0 can be released?

@b-reyes : yes let's get it in for v0.6.0. I can do this one (i.e. please assign it me!) once we have the beam dimension and the _alongship/athwart vs _major/minor variables in.

b-reyes commented 2 years ago

To bring the conversation back to the core discussion, there are two items we need a decision on for EK60.

To adhere to the convention, I suggest that we let beam_type have dims (channel, ping_time)

Since we are changing the dimensions of all the other similar variables from the raw dimension (channel) to be more compliant with the convention (channel, ping_time, beam), I kinda think we should keep this variable consistent with the other variables.

As these variables are not in the convention, we get decision making power here. @emiliom what do you think?

emiliom commented 2 years ago

To adhere to the convention, I suggest that we let beam_type have dims (channel, ping_time)

I agree. Sigh.

  • In regards to angle_offset_alongship/athwartship and angle_sensitivity_alongship/athwartship it appears that a larger discussion needs to occur for the particular names. For this issue, let’s keep the conversation to the dimensions of these variables. @leewujung commented the following:

Since we are changing the dimensions of all the other similar variables from the raw dimension (channel) to be more compliant with the convention (channel, ping_time, beam), I kinda think we should keep this variable consistent with the other variables.

As these variables are not in the convention, we get decision making power here. @emiliom what do you think?

I agree with assigning them (channel, ping_time, beam) dimensions.

(And just to elaborate on their similarity to related variables that are either in the convention or have analogs in the convention: those variables were missing ping_time as specified in the convention, so we're adding it; and, of course, we're also adding beam, since the variables do have beam in the convention)

b-reyes commented 2 years ago

Summary of dimension decisions for EK60. As @leewujung stated, “Here we are taking an approach of "addition" to only add dimensions to the variables if they do not already exist, but we DO NOT remove any dimensions from the convention. So, in practice, this means that we have a lot of identical values along the dimensions that are defined in the convention but are single values from the data file.”

EK60: Sonar/Beam_group1

b-reyes commented 2 years ago

Now that we have decided what the dimensions for EK60 will be, let’s continue our discussion with EK80. Most variables are similar to EK60 with some minor additions, but I have included all of them for future reference.

EK80: Sonar/Beam_group1/Sonar/Beam_group2

leewujung commented 2 years ago

Thanks for the summary @b-reyes, this is a huge job!

For EK80, everything you listed looks fine. My comments on the questions are below:

  • Do frequency_start/end correspond to transmit_frequency_start/stop in the convention? If so, then we should add beam as a dimension → only in Sonar/Beam_group1

Yes please add them. Agreed on adding the beam dimension.

  • What should be done with these variables, they do not appear in the convention?

    • beamwidth_twoway_alongship/athwartship

This is the same issue as the beamwidth_receive_alongship/athwartship and beamwidth_transmit_alongship/athwartship for EK60 -- as in the convention wants to store 1-way beamwidth, but the system only gives 2-way beamwidth. For encoding these, please use the same numbers in beamwidth_twoway_alongship/athwartship to save into beamwidth_receive_alongship/athwartship and beamwidth_transmit_alongship/athwartship . Let's take this in another issue (I will do this one, once everything else is settled).

  • slope

Slope can change ping by ping and be different for each channel, so the data are matrices (channel x ping_time) in the data file, so I suggest we save it with dim: (channel, ping_time) (i.e. we do not need to add the beam dimension)

  • transceiver_software_version

This does not change ping by ping in a single file, so I suggest saving it with dim: (channel).

There is in theory a possibility that someone decided to change the transceiver software in the middle a survey and later want to do combine_echodata for all the files. Without a ping_time dimension the combine operation wouldn't work. BUT I think in this case we should actually return an error and block that combine operation, since many things can change when the transceiver software is changed.

b-reyes commented 2 years ago
  • What should be done with these variables, they do not appear in the convention?

    • beamwidth_twoway_alongship/athwartship

This is the same issue as the beamwidth_receive_alongship/athwartship and beamwidth_transmit_alongship/athwartship for EK60 -- as in the convention wants to store 1-way beamwidth, but the system only gives 2-way beamwidth. For encoding these, please use the same numbers in beamwidth_twoway_alongship/athwartship to save into beamwidth_receive_alongship/athwartship and beamwidth_transmit_alongship/athwartship . Let's take this in another issue (I will do this one, once everything else is settled).

@leewujung I am currently writing up an issue for this, however, I wanted to clarify something. Based on the conversation with @gavinmacaulay, it seems like we should not "... use the same numbers in beamwidth_twoway_alongship/athwartship to save into beamwidth_receive_alongship/athwartship and beamwidth_transmit_alongship/athwartship" as you stated. Instead, we should multiply by 2 to go from 2-way to 1-way beamwidth. Is this correct or have I misunderstood something?

As this will be handled in a separate issue, should we leave these varaiables' dimensions unchanged for now?

  • slope

Slope can change ping by ping and be different for each channel, so the data are matrices (channel x ping_time) in the data file, so I suggest we save it with dim: (channel, ping_time) (i.e. we do not need to add the beam dimension)

Great, I will leave this variable unchanged.

  • transceiver_software_version

This does not change ping by ping in a single file, so I suggest saving it with dim: (channel).

There is in theory a possibility that someone decided to change the transceiver software in the middle a survey and later want to do combine_echodata for all the files. Without a ping_time dimension the combine operation wouldn't work. BUT I think in this case we should actually return an error and block that combine operation, since many things can change when the transceiver software is changed.

Sounds good, I will leave this variable unchanged also.

leewujung commented 2 years ago

@leewujung I am currently writing up an issue for this, however, I wanted to clarify something. Based on the conversation with @gavinmacaulay, it seems like we should not "... use the same numbers in beamwidth_twoway_alongship/athwartship to save into beamwidth_receive_alongship/athwartship and beamwidth_transmit_alongship/athwartship" as you stated. Instead, we should multiply by 2 to go from 2-way to 1-way beamwidth. Is this correct or have I misunderstood something?

As this will be handled in a separate issue, should we leave these varaiables' dimensions unchanged for now?

What I meant was that I will take care of the value mismatch in my later PR after your PR. Don't want you to worry about that right now (we'll circle back to the physics once we're done with this push). But please encode the variables with the correct dimensions in what we have discussed above, so that I don't end up making mistakes in my PR.

emiliom commented 2 years ago

For EK60:

transducer_offset_x/y/z(channel)

This variable was incorrectly found in the Beam_group1 group. It belongs in Platform and there's already a PR (#631) that makes that move. So, disregard the variable.

For EK80:

transceiver_software_version

@leewujung wouldn't this variable be better placed in the Sonar group, where the convention attribute sonar_software_version is already found? I suggest moving it there, and not adding a ping_time dimension at this time; let's leave that addition to a future echopype version.

BTW, I just noticed that in the sample EK80 raw data file I've been using (test_data/ek80/D20170912-T234910.raw), the Sonar.sonar_software_* attributes have been turned into variables with a frequency dimension. I can see why. If we want to discuss that, let's do it elsewhere.