Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add beam dimension to Beam group #520

Closed
2 tasks done
emiliom opened this issue Dec 15, 2021 · 56 comments
Closed
2 tasks done

Add beam dimension to Beam group #520

emiliom opened this issue Dec 15, 2021 · 56 comments
Assignees
Labels
data format Anything about data format
Milestone

Comments

@emiliom
Copy link
Collaborator

emiliom commented Dec 15, 2021

(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.

  • Currently for EK60 and AZFP, a beam dimension is not used explicitly and is effectively length 1. We'll need to add it.
  • Rename ek80 quadrant dimemsions and coordinates to beam #619 For EK80 there is already a dimension called quadrant that is conceptually equivalent (for our purposes) to the beam dimension as defined in SONAR-netCDF4 v1. We will need to rename it to beam.

Older notes (just for reference):

  • For single beam or split-beam, it could be a length-1 explicit dimension or an implicit coordinate (scalar beam )
  • For multi-beam, an explicit dimension is required. Currently the EK80 parser generates such a structure, but the dimension is named quadrant (WJ: there is an extra dimension quadrant — you can call them separate beams, but since Simrad use quadrant i used that for our first shot though I think it should be probably sector since it is not always 4)
  • EM: It occurs to me now that, in terms of match-up with SONAR-netCDF4, the Beam group in AZFP and EK60 echopype nc/zarr files could be described as having an implicit, single-value beam dimension. Single-value dimensions can be implemented interchangeably in a netcdf file either explicitly with the dimension or implicitly (refer to the CF convention). In SONAR-netCDF4, since there is an explicit beam dimension, single-beam data would be expected to be stored with beam as a dimension.
  • issue of convention use of beam dimension (ie, >1 beams per Beam subgroup? Based on "beam mode" alone?)
  • In the convention, under 2.10.6 (Sonar groups), it says: "Variable definitions for data from split-aperture systems are not currently specified"
@emiliom emiliom added the data format Anything about data format label Dec 15, 2021
@emiliom emiliom self-assigned this Dec 15, 2021
@emiliom emiliom added this to the 0.6.0 milestone Jan 6, 2022
@emiliom
Copy link
Collaborator Author

emiliom commented Feb 16, 2022

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
Copy link
Collaborator Author

emiliom commented Apr 11, 2022

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
Copy link
Collaborator Author

emiliom commented Apr 11, 2022

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
Copy link
Contributor

b-reyes commented Apr 12, 2022

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

  • What should the position of the dimension beam be for all sensors? For EK80, it is currently the 4th dimension e.g.
backscatter_i has dimensions (frequency, ping_time, range_sample, quadrant)
  • 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 and if we have four beams, quadrant = [0, 1, 2, 3]. Should we stick with this or make changes to be more consistent with the convention?

    • This will have some impact on @emiliom's comment:

    One thing I haven't looked into is what value the single-element (length 1) beam coordinate variable should have."

  • 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?

@emiliom
Copy link
Collaborator Author

emiliom commented Apr 12, 2022

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
Copy link
Member

leewujung commented Apr 12, 2022

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
Copy link
Contributor

b-reyes commented Apr 12, 2022

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

  • It appears that we should make beam the last dimension for all variables that contain it.
  • To determine which variables to add the beam dimension to, I should refer to the convention.

Items that still need to be addressed:

  • @leewujung brings up a good point, that integers are easier to reference with .isel(beam=1). However, as she also mentioned, if we want to replace the beam number with the beam name/ID, then we will need this dimension to be a string. Thus, the important thing to answer is, will we want to allow for the beam to be referenced by its actual name/ID?
    • How practical is it refer to a beam by name/ID? Do people usually know off the top of their head what this value actually is? Maybe a happy medium is to use integers for beam and then store the "conversion" as an attribute of the Sonar group (i.e. beam=1 refers to the beam name/ID blah).
  • For sensors that do no currently have the beam dimension, what do we want to set the default value as?

@b-reyes
Copy link
Contributor

b-reyes commented Apr 12, 2022

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.

  • For EK60 we only add beam_group_descr and beam_group_name to the Sonar group. Thus, should we also add these other variables to the other sensors in this PR?
  • These new data variables could be a perfect substitute for the "conversion" I mentioned above. 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) .

@emiliom
Copy link
Collaborator Author

emiliom commented Apr 12, 2022

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 😉.

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
Copy link
Collaborator Author

emiliom commented Apr 12, 2022

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
Copy link
Contributor

b-reyes commented Apr 12, 2022

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
Copy link
Member

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
Copy link
Contributor

b-reyes commented Apr 13, 2022

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
Copy link
Collaborator Author

emiliom commented Apr 13, 2022

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.

👍

@leewujung
Copy link
Member

I agree with @emiliom's comment above!

@b-reyes
Copy link
Contributor

b-reyes commented Apr 13, 2022

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

  • For all sensors we will let beam be a string and the elements will be integer numbers (that are strings).
    • These integer numbers will start at "1".
  • For the EK60, AZFP, and Beam_group2 in EK80 we will be adding the beam dimension with beam=["1"] to all variables that require it.
  • beam will be the last dimension for all variables that contain it.

@b-reyes
Copy link
Contributor

b-reyes commented Apr 14, 2022

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

  • Sonar/Beam_group1
    • beamwidth_receive_alongship and beamwidth_receive_athwartship
      • Add beam dim
    • beamwidth_transmit_alongship and beamwidth_transmit_athwartship
      • Add beam dim
    • beam_direction_x/y/z
      • Add beam for EK60 and AZFP
      • For EK80 Beam_group1, there is no beam dimension
    • angle_offset_alongship/athwartship
      • Add beam dim
    • angle_sensitivity_alongship/athwartship
      • Add beam dim
    • equivalent_beam_angle
      • Add beam dim for EK60
      • For EK80 this was not included
    • gain_correction
      • Add beam dim
    • backscatter_r
      • Add beam dim
    • sample_interval, data_type, count, transmit_mode
      • These are custom echopype variables and it looks like we should not add the beam dim
    • angle_athwartship and angle_alongship
      • Add beam dim
  • Vendor_specific
    • For echopype version 0.6.0, the consensus is to not add the beam dimension to any of these variables

@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
Copy link
Member

leewujung commented Apr 14, 2022

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
Copy link
Member

leewujung commented Apr 14, 2022

@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

  • Note that channel (the current frequency) dimension is an overarching echopype addition to make data easier to slice
  • Variables having dims (channel, ping_time, range_sample, beam)
    • backscatter_i/r
    • angle_alongship/athwartship
  • 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
    • beam_direction_x/y/z
    • angle_offset_alongship/athwartship
    • angle_sensitivity_alongship/athwartship
    • beamwidth_receive_alongship/athwartship
    • beamwidth_transmit_alongship/athwartship
    • equivalent_beam_angle
    • gain_correction
  • Variables having dims (channel, ping_time, beam) (channel, ping_time): these are matrices (dim: channel and ping_time) in the data file
    • sample_interval
    • data_type
    • count
    • transmit_mode

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
Copy link
Contributor

b-reyes commented Apr 14, 2022

@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)

  • Do not appear in the convention. Should we modify these?

    • channel_id
      • EP - (frequency)
    • gpt_software_version
      • EP - (frequency)
    • offset
      • EP - (frequency, ping_time)
  • Differs from convention. How do we want to treat these variables?

    • beam_type
      • Convention - (ping_time), EP - (frequency)
    • transmit_bandwidth
      • Convention - (ping_time), EP - (frequency, ping_time)
    • transmit_duration_nominal
      • Convention - (ping_time), EP - (frequency, ping_time)
    • transmit_power
      • Convention - (ping_time), EP - (frequency, ping_time)
  • 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)?

  • Appear in the convention without a dimension. Should we keep these variables as they are now?

    • transducer_offset_x/y/z
      • EP - (frequency)

@emiliom
Copy link
Collaborator Author

emiliom commented Apr 14, 2022

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
Copy link
Contributor

b-reyes commented Apr 14, 2022

@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?

  • To adhere to the convention, we should let beam_type have dims (channel, ping_time)
  • Current echopype variables that do not need their dimensions modified
    • transmit_bandwidth
    • transmit_duration_nominal
    • transmit_power
    • transducer_offset_x/y/z

@emiliom
Copy link
Collaborator Author

emiliom commented Apr 14, 2022

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

@b-reyes
Copy link
Contributor

b-reyes commented Apr 19, 2022

  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
Copy link
Collaborator Author

emiliom commented Apr 19, 2022

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
Copy link
Contributor

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
Copy link
Contributor

b-reyes commented Apr 19, 2022

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
Copy link
Member

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
Copy link
Contributor

b-reyes commented Apr 19, 2022

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

  • What should the dimensions of beam_type be? @gavinmacaulay made the following comment:

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.

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

  • 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?

@emiliom
Copy link
Collaborator Author

emiliom commented Apr 19, 2022

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
Copy link
Contributor

b-reyes commented Apr 19, 2022

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

  • Note that channel (the current frequency) dimension is an overarching echopype addition to make data easier to slice
  • Variables having dims (channel, ping_time, range_sample, beam)
    • backscatter_r
    • angle_alongship/athwartship
  • Variables having dims (channel, ping_time, beam): these are vectors (dim: channel) in the data file and in reality do not vary across ping_time
    • beam_direction_x/y/z
    • angle_offset_alongship/athwartship
    • angle_sensitivity_alongship/athwartship
    • beamwidth_receive_alongship/athwartship
    • beamwidth_transmit_alongship/athwartship
    • equivalent_beam_angle
    • gain_correction
  • Variables that do not need their dimensions modified
    • sample_interval(channel, ping_time)
    • data_type(channel, ping_time)
    • count(channel, ping_time)
    • transmit_mode(channel, ping_time)
    • transmit_bandwidth(channel, ping_time)
    • transmit_duration_nominal(channel, ping_time)l
    • transmit_power(channel, ping_time)
    • transducer_offset_x/y/z(channel)
    • gpt_software_version(channel)
    • offset(channel, ping_time)
  • The variable channel_id will be “removed” in the future as it will become our coordinate channel see Tracking issue for: rename dimensions in groups, including frequency to channel and *_time to time*, AZFP variable movements #566
  • To adhere to the convention we let beam_type have dims (channel, ping_time)

@b-reyes
Copy link
Contributor

b-reyes commented Apr 19, 2022

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

  • Variables having dims (channel, ping_time, range_sample, beam)
    • backscatter_i/rbackscatter_i is only in Sonar/Beam_group1
  • angle_alongship/athwartship → only in Sonar/Beam_group2
  • Variables having dims (channel, ping_time, beam): these are vectors (dim: channel) in the data file and in reality do not vary across ping_time
    • beam_direction_x/y/z
    • angle_offset_alongship/athwartship
    • angle_sensitivity_alongship/athwartship
    • equivalent_beam_angle
  • To adhere to the convention, we should let beam_type have dims (channel, ping_time)
  • Current echopype variables that do not need their dimensions modified
    • transducer_offset_x/y/z
    • transmit_duration_nominal
    • transmit_power
    • sample_interval
  • The variable channel_id will be “removed” in the future as it will become our coordinate channel see Tracking issue for: rename dimensions in groups, including frequency to channel and *_time to time*, AZFP variable movements #566
  • 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
  • What should be done with these variables, they do not appear in the convention?
    • beamwidth_twoway_alongship/athwartship
    • slope
    • transceiver_software_version

@leewujung
Copy link
Member

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
Copy link
Contributor

b-reyes commented Apr 20, 2022

  • 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
Copy link
Member

@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
Copy link
Collaborator Author

emiliom commented Apr 20, 2022

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.

@leewujung
Copy link
Member

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.

I agree with the above.

@b-reyes
Copy link
Contributor

b-reyes commented Apr 20, 2022

We have arrived at a consensus for EK80! Summary of dimension decisions for EK80.

EK80: Sonar/Beam_group1/Sonar/Beam_group2

  • Variables having dims (channel, ping_time, range_sample, beam)
    • backscatter_i/rbackscatter_i is only in Sonar/Beam_group1
  • angle_alongship/athwartship → only in Sonar/Beam_group2
  • 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
    • beam_direction_x/y/z
    • angle_offset_alongship/athwartship
    • angle_sensitivity_alongship/athwartship
    • equivalent_beam_angle
    • beamwidth_twoway_alongship/athwartship
  • To adhere to the convention, we will let beam_type have dims (channel, ping_time)
  • Current echopype variables that do not need their dimensions modified
    • transducer_offset_x/y/z(channel) Note: this will be moved to the Platform group
    • transmit_duration_nominal(channel, ping_time)
    • transmit_power(channel, ping_time)
    • sample_interval(channel, ping_time)
    • slope(channel, ping_time)
    • transceiver_software_version(channel) Note: this will be moved to the Sonar group
  • The variable channel_id will be “removed” in the future as it will become our coordinate channel see Tracking issue for: rename dimensions in groups, including frequency to channel and *_time to time*, AZFP variable movements #566
  • frequency_start/end(channel, ping_time) will have beam added as a dimension → only in Sonar/Beam_group1

@b-reyes
Copy link
Contributor

b-reyes commented Apr 20, 2022

Note that we will not be addressing the AD2CP sensor in this issue, this will be left for a future version of echopype. Thus, the last items to discuss are the AZFP sensor dimensions.

AZFP: Sonar/Beam_group1

  • Variables having dims (channel, ping_time, range_sample, beam)
    • backscatter_r
  • 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
    • equivalent_beam_angle
    • gain_correction
  • sample_interval should have dims (channel, ping_time), but currently echopype has it as just channel
  • For transmit_duration_nominal we should add ping_time as a dimension to be consistent convention (it currently only has channel as a dimension)
  • Variables not found in the convention, should we leave them alone?
    • temperature_counts(ping_time)
    • tilt_x/y_count(ping_time)
    • tilt_x/y(ping_time)
    • cos_tilt_mag(ping_time)
    • DS(channel)
    • EL(channel)
    • ​​TVR(channel)
    • VTX(channel)
    • Sv_offset(channel)
    • number_of_samples_digitized_per_pings(channel)
    • number_of_digitized_samples_averaged_per_pings(channel)

@leewujung
Copy link
Member

leewujung commented Apr 21, 2022

The dimensions for AZFP listed above all look good. I just have some comments below on where specific form of data come from, and placement of specific variables.

  • sample_interval should have dims (channel, ping_time), but currently echopype has it as just channel
  • For transmit_duration_nominal we should add ping_time as a dimension to be consistent convention (it currently only has channel as a dimension)

In the same data file AZFP does not have the flexibility like EK systems to change sample_interval or transmit_duration_nominal over time (hence only a channel dimension straight from the data), but we could run into situations when combine_echodata is used and we would need to accommodate having different sample_interval over time, so having dims (channel, ping_time) is a convenient change for both variables.

  • Variables not found in the convention, should we leave them alone?

I have some suggestions where the variables should be moved to. I was definitely not thinking about the variable placement earlier.

  • Move from Beam_groupX to Vendor group:
    • the variables below are specific to AZFP as engineering outputs, some can be converted to "real" data variables in the Environment or Platform groups
      • temperature_counts(ping_time)
      • tilt_x/y_count(ping_time)
      • tilt_x/y(ping_time)
      • cos_tilt_mag(ping_time)
    • the variables below are specific to AZFP and used for AZFP-specific calibration
      • DS(channel)
      • EL(channel)
      • ​​TVR(channel)
      • VTX(channel)
      • Sv_offset(channel)
      • number_of_samples_digitized_per_pings(channel)
      • number_of_digitized_samples_averaged_per_pings(channel)

@b-reyes
Copy link
Contributor

b-reyes commented Apr 22, 2022

Based on our Thursday meeting, we have come to a consensus on the variable dimensions of the beam group for AZFP. To summarize, here are the final decisions on the dimensions for AZFP:

AZFP: Sonar/Beam_group1

  • Variables having dims (channel, ping_time, range_sample, beam)
    • backscatter_r
  • Variables having dims (channel, ping_time, beam): these are vectors (dim: channel) in the data file and in reality do not vary across ping_time
    • equivalent_beam_angle
    • gain_correction
  • We will add ping_time to sample_interval and transmit_duration_nominal to comply with the convention. These variables will now have dims (channel, ping_time).
  • The following variables are not found in the convention and their dimensions will not be modified. Note that these variables will be moved to other groups, see Move some AZFP Beam variables to Vendor and Platform groups #642.
    • temperature_counts(ping_time)
    • tilt_x/y_count(ping_time)
    • tilt_x/y(ping_time)
    • cos_tilt_mag(ping_time)
    • DS(channel)
    • EL(channel)
    • ​​TVR(channel)
    • VTX(channel)
    • Sv_offset(channel)
    • number_of_samples_digitized_per_pings(channel)
    • number_of_digitized_samples_averaged_per_pings(channel)

@leewujung
Copy link
Member

We are ready to close this now. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data format Anything about data format
Projects
Status: Done
Development

No branches or pull requests

4 participants