Support frequency nominal in consolidate functions#1734
Conversation
|
@LOCEANlloydizard @leewujung I think this is ready to review. I think I addressed all the comments from the previous PR, but let me know if I didn't catch something. |
| dim_0 = get_dim_0(ds) | ||
| # Identify beam group name by checking `dim_0` values of `ds` | ||
| if echodata["Sonar/Beam_group1"][dim_0].equals(ds[dim_0]): | ||
| beam_group_name = "Beam_group1" | ||
| else: | ||
| beam_group_name = "Beam_group2" |
There was a problem hiding this comment.
I think this logic would no longer work with the ping sequencing capability on WBAT, since the same channel can be configured to transmit both FM and CW signals and record in either complex or power-angle format. On WBT this would be fine since each ping library entry will produce a _1, _2, etc postfix as the channel_id.
Is there another way we can find the right channel beam directions from the echodata object?
How about using the channel_all and beam_group_index variables in the Sonar group that we added to help index the beam group to calibrate?
| # Select beam group based on dim_0 | ||
| ds_beam = echodata[ed_beam_group].sel({dim_0: source_Sv[dim_0].values}) |
There was a problem hiding this comment.
Same as my comment above, this logic would not work and needs to be updated.
leewujung
left a comment
leewujung
left a comment
There was a problem hiding this comment.
Hey @ctuguinay : Thanks for sorting through the previous PR and consolidating the changes to this new one! I know you didn't write the previous code but please help with the following requested changes.
I see a few .sel({dim_0: dim_0_id} usages which try to use whatever dim_0 is (either frequency_nominal or channel) to slice datasets or dataarrays. I think a better way is to use get_dim_0 to find out what dim_0 is, and if it is frequency_nominal, map it back to channel using what's in Sonar/channel_all, so that all the indexing is done using channel. This is because we align pretty everything in our code to be the channel_if, and in view of #1721, it's better to just keep all indexing using channel to future proof the code.
I also suggest the following to keep things a bit tidier:
- rename
loc_utils.pytoutils_loc.py - move
get_dim_0toutils_loc.py - rename
ek_depth_utils.pytoutils._ek_depth.py
One of my comments requires a bit of consideration to figure out a good solution. Let me know if you want to bounce ideas.
…r add depth based on waveform_mode and encode_mode
…r use beam angles and sonar model ek80
|
@leewujung This should be ready for another look 🙏 |
2 participants
Additionally enforces that channel or frequency nominal exists in all datasets passed into the consolidate functions.
This is a slight modification of #1520 by @dbashford-NOAA without code conflicts with the more recent changes.
When merged, it closes #1488.