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

Fix regression bug in computing MVBS in selecting echo_range for specific frequency #736

Merged
merged 4 commits into from
Jul 1, 2022
Merged

Fix regression bug in computing MVBS in selecting echo_range for specific frequency #736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be009b3
minor docstring change
leewujung Jun 21, 2022
a8261a5
fix bug from the process of converting freq to ch
leewujung Jun 21, 2022
77086ff
reorder dimensions for AZFP Sv echo_range to be identical with Sv
leewujung Jun 21, 2022
b6205e5
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 21, 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
3 changes: 3 additions & 0 deletions echopype/calibrate/calibrate_azfp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ def _cal_power(self, cal_type, **kwargs):
# Add env and cal parameters
out = self._add_params_to_output(out)

# Order the dimensions
out["echo_range"] = out["echo_range"].transpose("channel", "ping_time", "range_sample")

Comment on lines +157 to +159
Copy link
Contributor

Choose a reason for hiding this comment

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

@leewujung I am wondering if this is the most appropriately place for this. This line is necessary because of how we create the EchoData object for AZFP, correct? If so, should we instead correct the order in set_groups_azfp? I am fine with leaving this as is, I just thought I would bring this up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I agree this is the best done in set_groups_azfp. In fact this was part of my test to see if we can do without .drop("channel") in this line:

echopype/echopype/preprocess/api.py

Line 79 in b6205e5

ds["echo_range"].isel(ping_time=0).squeeze().drop("channel").data,

Somehow that extra dimension/coordinate only shows up for AZFP data, and I was wondering if it had anything to do with the order of the dimension, but that wasn't the reason.

I'll create an issue so we remember to do this in set_groups_azfp instead of here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh actually, now that I looked at this again, the order of dimension to be changed is for echo_range and not Sv (which came from backscatter_r), so I guess nothing has to change for set_groups_azfp, but it is the AZFP part of compute_range that needs to change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was having a little trouble tracking down where that happened. The AZFP part of compute_range was using parts of the EchoData object, so I assumed that we needed to make corrections in set_groups_azfp. After looking into this a little further, I found that in the EK else statement of compute_range we do range_meter = range_meter.transpose("channel", "ping_time", "range_sample"), so maybe the fix is as simple as adding that to the AZFP if statement block.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right, cool that you tracked it down! I'll link your comment to that issue... v0.6.2... so that we don't have to do this now. :P

# Squeeze out the beam dim
# doing it here because both out and self.cal_params["equivalent_beam_angle"] has beam dim
return out.squeeze("beam", drop=True)
Expand Down
36 changes: 17 additions & 19 deletions echopype/preprocess/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,18 @@


def _check_range_uniqueness(ds):
"""Check if range (``echo_range``) changes across ping in a given frequency channel."""
"""
Check if range (``echo_range``) changes across ping in a given frequency channel.
"""
return (
ds["echo_range"].isel(ping_time=0).dropna(dim="range_sample")
== ds["echo_range"].dropna(dim="range_sample")
).all()


def _set_MVBS_attrs(ds):
"""Attach common attributes
"""
Attach common attributes.

Parameters
----------
Expand All @@ -42,7 +45,8 @@ def _set_MVBS_attrs(ds):


def compute_MVBS(ds_Sv, range_meter_bin=20, ping_time_bin="20S"):
"""Compute Mean Volume Backscattering Strength (MVBS)
"""
Compute Mean Volume Backscattering Strength (MVBS)
based on intervals of range (``echo_range``) and ``ping_time`` specified in physical units.

Output of this function differs from that of ``compute_MVBS_index_binning``, which computes
Expand All @@ -68,30 +72,23 @@ def compute_MVBS(ds_Sv, range_meter_bin=20, ping_time_bin="20S"):
"echo_range variable changes across pings in at least one of the frequency channels."
)

# TODO: right now this computation is brittle as it takes echo_range
# from only the lowest frequency to make it the range for all channels.
# This should be implemented different to allow non-uniform echo_range.

# get indices of sorted frequency_nominal values. This is necessary
# because the frequency_nominal values are not always in ascending order.
sorted_freq_ind = np.argsort(ds_Sv.frequency_nominal)

def _freq_MVBS(ds, rint, pbin):
sv = 10 ** (ds["Sv"] / 10) # average should be done in linear domain
sv.coords["range_meter"] = (
sv.coords["echo_range"] = (
["range_sample"],
ds_Sv["echo_range"].isel(channel=sorted_freq_ind[0], ping_time=0).data,
ds["echo_range"].isel(ping_time=0).squeeze().drop("channel").data,
)
sv = sv.swap_dims({"range_sample": "range_meter"})
sv = sv.swap_dims({"range_sample": "echo_range"})
sv_groupby_bins = (
sv.groupby_bins("range_meter", bins=rint, right=False, include_lowest=True)
sv.groupby_bins("echo_range", bins=rint, right=False, include_lowest=True)
.mean()
.resample(ping_time=pbin, skipna=True)
.mean()
)
sv_groupby_bins.coords["echo_range"] = (["range_meter_bins"], rint[:-1])
sv_groupby_bins = sv_groupby_bins.swap_dims({"range_meter_bins": "echo_range"})
sv_groupby_bins = sv_groupby_bins.drop_vars("range_meter_bins")
sv_groupby_bins.coords["echo_range"] = (["echo_range_bins"], rint[:-1])
sv_groupby_bins = sv_groupby_bins.swap_dims({"echo_range_bins": "echo_range"}).drop_vars(
"echo_range_bins"
)
return 10 * np.log10(sv_groupby_bins)

# Groupby freq in case of different echo_range (from different sampling intervals)
Expand Down Expand Up @@ -161,7 +158,8 @@ def _freq_MVBS(ds, rint, pbin):


def compute_MVBS_index_binning(ds_Sv, range_sample_num=100, ping_num=100):
"""Compute Mean Volume Backscattering Strength (MVBS)
"""
Compute Mean Volume Backscattering Strength (MVBS)
based on intervals of ``range_sample`` and ping number (``ping_num``) specified in index number.

Output of this function differs from that of ``compute_MVBS``, which computes
Expand Down