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.

Sign up for GitHub

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

Overhaul and fix Sonar beam_group variables #658

Merged
merged 9 commits into from
May 10, 2022
Merged

Overhaul and fix Sonar beam_group variables #658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
11758c1
Corrected Sonar beam_group_ variable assignments to support EK80 1- v…
emiliom May 1, 2022
107ad22
Renamed Sonar beam_group_name var to beam_group and made it the missi…
emiliom May 1, 2022
7c91299
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 1, 2022
20080a3
Implement beamgroups_possible class variable in all SetGroups classes
emiliom May 5, 2022
37cbe7a
Merge branch 'dev' into beamgroups_sonarvars
emiliom May 5, 2022
cbbd227
add beam_group coord to ek80 sonar group
b-reyes May 6, 2022
91dc06d
Update echopype/convert/set_groups_ek80.py
emiliom May 6, 2022
2706cfe
Update echopype/convert/set_groups_ek80.py
emiliom May 10, 2022
62912fa
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions echopype/convert/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,19 +449,26 @@ def open_raw(
if sonar_model in ["EK60", "ES70", "EK80", "ES80", "EA640"]:
tree_dict["Platform/NMEA"] = setgrouper.set_nmea()
tree_dict["Provenance"] = setgrouper.set_provenance()
tree_dict["Sonar"] = setgrouper.set_sonar()
# Allocate a tree_dict entry for Sonar? Otherwise, a DataTree error occurs
tree_dict["Sonar"] = None

# Set multi beam groups
beam_groups = setgrouper.set_beam()
if isinstance(beam_groups, xr.Dataset):
# if it's a single dataset like the ek60,
# make into list
# if it's a single dataset like the ek60, make into list
beam_groups = [beam_groups]

valid_beam_groups_count = 0
for idx, beam_group in enumerate(beam_groups, start=1):
if beam_group is not None:
valid_beam_groups_count += 1
tree_dict[f"Sonar/Beam_group{idx}"] = beam_group

if sonar_model in ["EK80", "ES80", "EA640"]:
tree_dict["Sonar"] = setgrouper.set_sonar(beam_group_count=valid_beam_groups_count)
else:
tree_dict["Sonar"] = setgrouper.set_sonar()

tree_dict["Vendor"] = setgrouper.set_vendor()

# Create tree and echodata
Expand Down
58 changes: 35 additions & 23 deletions echopype/convert/set_groups_ad2cp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,23 @@ def merge_attrs(datasets: List[xr.Dataset]) -> List[xr.Dataset]:


class SetGroupsAd2cp(SetGroupsBase):
"""Class for saving groups to netcdf or zarr from Ad2cp data files."""

beamgroups_possible = [
{
"name": "Beam_group1",
"descr": (
"contains velocity, correlation, and backscatter power (uncalibrated)"
" data and other data derived from acoustic data."
),
}
]

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.pulse_compressed = self.parser_obj.get_pulse_compressed()
self.combine_packets()

self._beamgroups = [
{
"name": "Beam_group1",
"descr": (
"contains velocity, correlation, and backscatter power (uncalibrated)"
" data and other data derived from acoustic data."
),
}
]

def combine_packets(self):
self.ds = None

Expand Down Expand Up @@ -368,22 +370,32 @@ def set_vendor(self) -> xr.Dataset:
return set_encodings(ds)

def set_sonar(self) -> xr.Dataset:
ds = xr.Dataset(
attrs={
"sonar_manufacturer": "Nortek",
"sonar_model": "AD2CP",
"sonar_serial_number": "",
"sonar_software_name": "",
"sonar_software_version": "",
"sonar_firmware_version": "",
"sonar_type": "acoustic Doppler current profiler (ADCP)",
}
)
"""Set the Sonar group."""

# Add beam_group and beam_group_descr variables sharing a common dimension
# (beam_group), using the information from self._beamgroups
self._beamgroups = self.beamgroups_possible
beam_groups_vars, beam_groups_coord = self._beam_groups_vars()
ds = xr.Dataset(beam_groups_vars, coords=beam_groups_coord)

# Assemble sonar group dictionary
sonar_dict = {
"sonar_manufacturer": "Nortek",
"sonar_model": "AD2CP",
"sonar_serial_number": "",
"sonar_software_name": "",
"sonar_software_version": "",
"sonar_firmware_version": "",
"sonar_type": "acoustic Doppler current profiler (ADCP)",
}
if "serial_number" in self.ds:
ds.attrs["sonar_serial_number"] = int(self.ds["serial_number"].data[0])
sonar_dict["sonar_serial_number"] = int(self.ds["serial_number"].data[0])
firmware_version = self.parser_obj.get_firmware_version()
if firmware_version is not None:
ds.attrs["sonar_firmware_version"] = ", ".join(
sonar_dict["sonar_firmware_version"] = ", ".join(
[f"{k}:{v}" for k, v in firmware_version.items()]
)

ds = ds.assign_attrs(sonar_dict)

return ds
22 changes: 12 additions & 10 deletions echopype/convert/set_groups_azfp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,16 @@ class SetGroupsAZFP(SetGroupsBase):
# Variables that need beam and ping_time dimensions added to them.
beam_ping_time_names = {"equivalent_beam_angle", "gain_correction"}

beamgroups_possible = [
{
"name": "Beam_group1",
"descr": "contains backscatter power (uncalibrated) and other beam or channel-specific data.", # noqa
}
]

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

self._beamgroups = [
{
"name": "Beam_group1",
"descr": "contains backscatter power (uncalibrated) and other beam or channel-specific data.", # noqa
}
]

def set_env(self) -> xr.Dataset:
"""Set the Environment group."""
# TODO Look at why this cannot be encoded without the modifications
Expand Down Expand Up @@ -66,9 +66,11 @@ def set_env(self) -> xr.Dataset:
def set_sonar(self) -> xr.Dataset:
"""Set the Sonar group."""

# Add beam_group_name and beam_group_descr variables sharing a common dimension (beam),
# using the information from self._beamgroups
ds = xr.Dataset(self._beam_groups_vars())
# Add beam_group and beam_group_descr variables sharing a common dimension
# (beam_group), using the information from self._beamgroups
self._beamgroups = self.beamgroups_possible
beam_groups_vars, beam_groups_coord = self._beam_groups_vars()
ds = xr.Dataset(beam_groups_vars, coords=beam_groups_coord)

# Assemble sonar group dictionary
sonar_dict = {
Expand Down
18 changes: 10 additions & 8 deletions echopype/convert/set_groups_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,22 +187,24 @@ def _parse_NMEA(self):
return time1, msg_type, lat, lon

def _beam_groups_vars(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emiliom I don't think this function correctly accounts for the EK80 sensor. From my understanding, there do exist EK80 files that only have the power data. Thus, these types of files would have Beam_group1 with the description "contains complex backscatter data and other beam or channel-specific data". If you want a file that exhibits this behavior, please see echopype/test_data/ek80_new/D20211004-T234429.raw.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate? I think what you're saying is that the current group description for EK80 Beam_group1 is inadequate. If that's the case, then the fix would be to that text in SetGroupsEK80 (note that @leewujung composed that text). This function is generic and just sets up the beam_groups_descr variable and beam_group coord variable, based on the beam group information that's passed to it.

Copy link
Contributor

@b-reyes b-reyes May 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the description is fine, just how you are assigning it to the beam group is incorrect. This is a known possibility, in 1.0.yml we have

name: Beam_group1
    description: >-
      contains backscatter data (either complex samples or uncalibrated power samples)
      and other beam or channel-specific data, including split-beam angle data when they exist.

Note that we say either complex samples OR uncalibrated power samples

Basically, for EK80, your code should check if only one beam group exists and if only one beam group exists, then it should select the appropriate description. It seems like the issue here (at least for EK80) is that beamgroups_possible ties the description to a specific beam group i.e. Beam_group1 is always for the complex backscatter data. In practice, Beam_group1 can correspond to backscatter data that are complex samples OR power samples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Let me chew on this. I'll get back to it mid-late afternoon, hopefully.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've looked at this more closely. For the sample file you listed (test_data/ek80_new/D20211004-T234429.raw -- thanks!), the data is written to Beam_group1 and its description becomes the EK80 Beam_group1 description text we have, "contains complex backscatter data and other beam or channel-specific data." I'm not sure if you were saying that that's what should happen but doesn't or what does happen but shouldn't, but I believe you're saying the latter. That's the intended behavior, and not something that was changed in this PR. I think @leewujung provided that EK80 beam group description text.

I still think that what you're getting at can be addressed by simply changing the description in beamgroups_possible that's associated with Beam_group1. It sounds like something like this would be more appropriate: "contains backscatter data (either complex samples or uncalibrated power samples) and other beam or channel-specific data".

Also, just to narrow down the problem, the function beam_groups_vars itself would not be the source of the potential problem, or solution. It's very generic. It just writes out what's passed to it. The issue you're getting at is what's passed to it by SetGroupsEK80.set_sonar, which occurs here:

echopype/echopype/convert/set_groups_ek80.py

Lines 171 to 172 in 91dc06d

self._beamgroups = self.beamgroups_possible[:beam_group_count]
beam_groups_vars, beam_groups_coord = self._beam_groups_vars()

The code does check -- elsewhere -- if only one beam group exists; if that's the case, that group will be Beam_group1 and the corresponding dict from beamgroups_possible is passed to it. The description text for that group doesn't need to exactly specify whether the beam group is complex or power only; that would be nice, and we could look into it in the future, but it's not necessary. That description does need to be accurate and not misleading, though. It sounds like a change to that description along the lines of what I suggest would make it more accurate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've looked at this more closely. For the sample file you listed (test_data/ek80_new/D20211004-T234429.raw -- thanks!), the data is written to Beam_group1 and its description becomes the EK80 Beam_group1 description text we have, "contains complex backscatter data and other beam or channel-specific data." I'm not sure if you were saying that that's what should happen but doesn't or what does happen but shouldn't, but I believe you're saying the latter. That's the intended behavior, and not something that was changed in this PR. I think @leewujung provided that EK80 beam group description text.

Yes, I was saying the latter. For the file you reference (and I provided), there is no complex backscatter data, so the description is wrong.

I still think that what you're getting at can be addressed by simply changing the description in beamgroups_possible that's associated with Beam_group1. It sounds like something like this would be more appropriate: "contains backscatter data (either complex samples or uncalibrated power samples) and other beam or channel-specific data".

Although your change to the description does fix the issue I described, it somewhat conflicts with the Beam_group2 description: "contains backscatter power (uncalibrated) and other beam or channel-specific data, including split-beam angle data when they exist." I am sure any user could infer that Beam_group1 corresponds to "complex samples" when two beam groups exist. However, I liked the idea of being very specific and stating whether the beam group has complex or power only data. I think this would be simple to implement and would benefit the user by providing further clarity. If you think this change to a more specific description conflicts with the intent of this PR, I understand. I am willing to take on this issue, if you do not have the time for it.

Also, just to narrow down the problem, the function beam_groups_vars itself would not be the source of the potential problem, or solution. It's very generic. It just writes out what's passed to it. The issue you're getting at is what's passed to it by SetGroupsEK80.set_sonar

I see where you are coming from, maybe the function beam_groups_vars is not the most appropriate place to change this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I liked the idea of being very specific and stating whether the beam group has complex or power only data. I think this would be simple to implement and would benefit the user by providing further clarity. If you think this change to a more specific description conflicts with the intent of this PR, I understand. I am willing to take on this issue, if you do not have the time for it.

If you really think it'd be very simple to implement, go ahead; otherwise my vote would be for leaving it to 0.6.1. While the impact of this addition is "minor" on the scope of this PR (and it does fit its intent), I think it should be done in a different PR, mainly so we can move on with this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "simple" was the wrong word. I would say it is doable haha. I am fine if you would like to leave this for 0.6.1. I think we have enough on our plate for 0.6.0. I will create a new issue for this. For this PR, we can just change the Beam_group1 description to "contains backscatter data (either complex samples or uncalibrated power samples) and other beam or channel-specific data", as you suggested.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine if you would like to leave this for 0.6.1. I think we have enough on our plate for 0.6.0. I will create a new issue for this.

That would be perfect, thank you. If you already had concrete ideas on how to accomplish this, I suggest you describe them at least briefly when creating the new issue, while they're fresh in your mind.

For this PR, we can just change the Beam_group1 description to "contains backscatter data (either complex samples or uncalibrated power samples) and other beam or channel-specific data", as you suggested.

I'll go ahead and make the change now.

"""Stage beam_group_name and beam_group_descr variables sharing a common dimension,
beam_group, to be inserted in the Sonar group"""
"""Stage beam_group coordinate and beam_group_descr variables sharing
a common dimension, beam_group, to be inserted in the Sonar group"""
beam_groups_vars = {
"beam_group_name": (
["beam_group"],
[di["name"] for di in self._beamgroups],
{"long_name": "Beam group name"},
),
"beam_group_descr": (
["beam_group"],
[di["descr"] for di in self._beamgroups],
{"long_name": "Beam group description"},
),
}
beam_groups_coord = {
"beam_group": (
["beam_group"],
[di["name"] for di in self._beamgroups],
{"long_name": "Beam group name"},
),
}

return beam_groups_vars
return beam_groups_vars, beam_groups_coord

def _add_beam_dim(
self, ds: xr.Dataset, beam_only_names: Set[str], beam_ping_time_names: Set[str]
Expand Down
28 changes: 15 additions & 13 deletions echopype/convert/set_groups_ek60.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,19 @@ class SetGroupsEK60(SetGroupsBase):
"gain_correction",
}

beamgroups_possible = [
{
"name": "Beam_group1",
"descr": (
"contains backscatter power (uncalibrated) and other beam or"
" channel-specific data, including split-beam angle data when they exist."
),
}
]

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

self._beamgroups = [
{
"name": "Beam_group1",
"descr": (
"contains backscatter power (uncalibrated) and other beam or"
" channel-specific data, including split-beam angle data when they exist."
),
}
]

self.old_ping_time = None
# correct duplicate ping_time
for ch in self.parser_obj.config_datagram["transceivers"].keys():
Expand Down Expand Up @@ -182,9 +182,11 @@ def set_env(self) -> xr.Dataset:
def set_sonar(self) -> xr.Dataset:
"""Set the Sonar group."""

# Add beam_group_name and beam_group_descr variables sharing a common dimension (beam),
# using the information from self._beamgroups
ds = xr.Dataset(self._beam_groups_vars())
# Add beam_group and beam_group_descr variables sharing a common dimension
# (beam_group), using the information from self._beamgroups
self._beamgroups = self.beamgroups_possible
beam_groups_vars, beam_groups_coord = self._beam_groups_vars()
ds = xr.Dataset(beam_groups_vars, coords=beam_groups_coord)

# Assemble sonar group dictionary
sonar_dict = {
Expand Down
44 changes: 25 additions & 19 deletions echopype/convert/set_groups_ek80.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,26 @@ class SetGroupsEK80(SetGroupsBase):
"beamwidth_twoway_athwartship",
}

beamgroups_possible = [
{
"name": "Beam_group1",
"descr": (
"contains backscatter data (either complex samples or uncalibrated power samples)" # noqa
" and other beam or channel-specific data"
),
},
{
"name": "Beam_group2",
"descr": (
"contains backscatter power (uncalibrated) and other beam or channel-specific data," # noqa
" including split-beam angle data when they exist."
),
},
]

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

self._beamgroups = [
{
"name": "Beam_group1",
"descr": "contains complex backscatter data and other beam or channel-specific data.", # noqa
},
{
"name": "Beam_group2",
"descr": (
"contains backscatter power (uncalibrated) and other beam or channel-specific data," # noqa
" including split-beam angle data when they exist."
),
},
]

def set_env(self, env_only=False) -> xr.Dataset:
"""Set the Environment group."""
# If only saving environment group,
Expand Down Expand Up @@ -150,7 +153,7 @@ def set_env(self, env_only=False) -> xr.Dataset:
)
return set_encodings(ds)

def set_sonar(self) -> xr.Dataset:
def set_sonar(self, beam_group_count=1) -> xr.Dataset:
# Collect unique variables
params = [
"transducer_frequency",
Expand All @@ -166,9 +169,11 @@ def set_sonar(self) -> xr.Dataset:
var[param].append(data[param])

# Create dataset
# beam_group_name and beam_group_descr variables sharing a common dimension (beam),
# using the information from self._beamgroups
beam_groups_vars = self._beam_groups_vars()
# Add beam_group and beam_group_descr variables sharing a common dimension
# (beam_group), using the information from self._beamgroups
self._beamgroups = self.beamgroups_possible[:beam_group_count]
beam_groups_vars, beam_groups_coord = self._beam_groups_vars()

sonar_vars = {
"frequency_nominal": (
["channel"],
Expand All @@ -194,7 +199,8 @@ def set_sonar(self) -> xr.Dataset:
["channel"],
list(self.parser_obj.config_datagram["configuration"].keys()),
self._varattrs["beam_coord_default"]["channel"],
)
),
**beam_groups_coord,
},
attrs={"sonar_manufacturer": "Simrad", "sonar_type": "echosounder"},
)
Expand Down
2 changes: 1 addition & 1 deletion echopype/echodata/convention/1.0.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ groups:
description: >-
contains backscatter power (uncalibrated) and other beam or channel-specific data,
including split-beam angle data when they exist.
Only exists if complex backscatter data they already in Sonar/Beam_group1
Only exists if complex backscatter data are already in Sonar/Beam_group1
ep_group: Sonar/Beam_group2
vendor:
name: Vendor specific
Expand Down