Create more specific beam group descriptions for EK80

[b-reyes]

In PR #658 it was found that the beam group descriptions for EK80 can become more specific.

As it stands right now, we let the Beam_group1 description be "contains backscatter data (either complex samples or uncalibrated power samples) and other beam or channel-specific data".

To be more explicit, we can let the Beam_group1 description be:

• When only power data exists:
  • "contains backscatter power (uncalibrated) and other beam or channel-specific data."
• When complex samples exist:
  • "contains complex backscatter data and other beam or channel-specific data."

I have a rough idea of how we can accomplish this.

1. Modify beamgroups_possible (in EK80) to:

beamgroups_possible = [
        {
            "name": "Beam_group1",
            "descr":
                {
                    'power': "contains backscatter power (uncalibrated) and "
                             "other beam or channel-specific data.",
                    'complex': "contains complex backscatter data 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."
            ),
        },
    ]

2. Replace

echopype/echopype/convert/api.py

Lines 461 to 470 in e0b3cbf

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

with

    beam_group_type = []
    for idx, beam_group in enumerate(beam_groups, start=1):
        if beam_group is not None:
            beam_group_type.append("complex" if "backscatter_i" in beam_group else "power")
            tree_dict[f"Sonar/Beam_group{idx}"] = beam_group


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

3. In set_sonar (for EK80) let

(

echopype/echopype/convert/set_groups_ek80.py

Line 174 in e0b3cbf

self._beamgroups = self.beamgroups_possible[:beam_group_count]

)

become

beam1_val = beamgroups_possible[0]
beam1_val['descr'] = beam1_val['descr']['complex'] 
self._beamgroups = [beam1_val] + beamgroups_possible[1:]

@emiliom what do you think?

[emiliom]

Thanks @b-reyes ! Sorry, I won't comment on it until we're on the other side of 0.6.0.

Note: this issue is related to #625

[leewujung]

Related to #719 and its upcoming implementation which will include changes like what's proposed above but for AD2CP.

[leewujung]

@b-reyes : could you take care of this and #625? The description are very similar and probably is a duplication or heavily overlap. Thanks.

[b-reyes]

@b-reyes : could you take care of this and #625? The description are very similar and probably is a duplication or heavily overlap. Thanks.

I have looked back into this and it seems this issue is specifically discussing changes to the beam group descriptions. On the other hand, #625 is more concerned with where the repr pulls the beam group descriptions from.

To proceed with this issue, I think it is good to have @emiliom review my proposed solution above. If he thinks the solution is appropriate, I will implement it. In the meantime, I can do a PR for #625.

[emiliom]

Yeah, this issue is specifically about the beam group description text that is inserted into the EchoData object during open_raw, for EK80.

@b-reyes I think your solution is mostly there. It looks like the very last part, the change to set_groups_ek80.set_sonar, needs some work. First, obviously, the method argument needs to change to something like this:

set_sonar(self, beam_group_type=['complex', 'power']) -> xr.Dataset:

(I'm not sure about the argument default value)

Then, the final code block you proposed:

beam1_val = beamgroups_possible[0]
beam1_val['descr'] = beam1_val['descr']['complex'] 
self._beamgroups = [beam1_val] + beamgroups_possible[1:]

doesn't use the new beam_group_type argument you've introduced and doesn't handle the case where beam group 2 is not present. But I think you have all the pieces in place to handle that case by using beam_group_type.

I'll say that, stylistically, I don't love the idea that this change would make the two beamgroups_possible dicts different in structure, and different from what's in the other sonar models. Now, the descr item value will be a dict for beam group 1 and a string for beam group 2 (the latter being the same in other sonar models). Currently I don't think there's anything that relies on these dicts having the same structure across the board. But overall consistency across sonar models is very helpful, if nothing else, in minimizing cognitive overload.

Regarding #625, while the focus there is on the yaml file and how it's used (in the repr), that issue is related to this issue to the extent that we'll have to decide where the repr gets the beam_group descriptions from. This issue (#671) is focused entirely on what happens on open_raw. The repr handling also needs to work on EchoData objects generated via open_converted. But, that can be discussed within #625.

[b-reyes]

doesn't use the new beam_group_type argument you've introduced and doesn't handle the case where beam group 2 is not present. But I think you have all the pieces in place to handle that case by using beam_group_type.

Yes, it does appear that I have not properly handled these cases. If we agree with this beamgroups_possible form, I will make changes to that code snippet.

I'll say that, stylistically, I don't love the idea that this change would make the two beamgroups_possible dicts different in structure, and different from what's in the other sonar models. Now, the descr item value will be a dict for beam group 1 and a string for beam group 2 (the latter being the same in other sonar models). Currently I don't think there's anything that relies on these dicts having the same structure across the board. But overall consistency across sonar models is very helpful, if nothing else, in minimizing cognitive overload.

I agree that this is not ideal. Do you have a better solution in mind?

Regarding #625, while the focus there is on the yaml file and how it's used (in the repr), that issue is related to this issue to the extent that we'll have to decide where the repr gets the beam_group descriptions from. This issue (#671) is focused entirely on what happens on open_raw. The repr handling also needs to work on EchoData objects generated via open_converted. But, that can be discussed within #625.

From my understanding we will be getting the beam_group descriptions from ed['Sonar'].beam_group_descr (see comment). If we agree that this is where the descriptions will come from, then these two issues are disconnected (as we are currently getting the descriptions from the yaml) and open_converted should be handled correctly.

[emiliom]

Yes, it does appear that I have not properly handled these cases. If we agree with this beamgroups_possible form, I will make changes to that code snippet.

Well, I don't have an alternative suggestion, so I guess that's the way forward!

I agree that this is not ideal. Do you have a better solution in mind?

See above 😉 . All I can think of is to reproduce the new dict structure you're proposing for beamgroups_possible with the other sonar models. That would maintain consistency, at the cost of verbosity in the code. It doesn't seem worth it.

From my understanding we will be getting the beam_group descriptions from ed['Sonar'].beam_group_descr (see #625 (comment)). If we agree that this is where the descriptions will come from, then these two issues are disconnected (as we are currently getting the descriptions from the yaml) and open_converted should be handled correctly.

Right! I'd read that but hadn't fully assimilated what you were saying. I'm on board.

[b-reyes]

Well, I don't have an alternative suggestion, so I guess that's the way forward!
All I can think of is to reproduce the new dict structure you're proposing for beamgroups_possible with the other sonar models. That would maintain consistency, at the cost of verbosity in the code. It doesn't seem worth it.

@emiliom sounds good. Based on your comments, to address this issue (#671) we will use the above form for beamgroups_possible for EK80 only and keep the same form for the rest of the sonar models. If this is the case, I can start working on this issue and I will make sure to take into account the beam group type in the final block of my suggested code above.

Right! I'd read that but hadn't fully assimilated what you were saying. I'm on board.

@emiliom it seems like we have also arrived at a decision for #625 here. To summarize, we will get the beam_group descriptions from ed['Sonar'].beam_group_descr when constructing the repr.

[emiliom]

Based on your comments, to address this issue (#671) we will use the above form for beamgroups_possible for EK80 only and keep the same form for the rest of the sonar models.

Yes.

it seems like we have also arrived at a decision for #625 here. To summarize, we will get the beam_group descriptions from ed['Sonar'].beam_group_descr when constructing the repr.

Yes, but let's shift that conversation back to #625, where it belongs.

[leewujung]

This is now addressed in #671 .