Add attributes for Sp and MVBS from index binning #615

emiliom · 2022-04-06T07:58:22Z

Addresses remaining tasks in #582 and #481

for more information, see https://pre-commit.ci

emiliom · 2022-04-06T15:02:16Z

echopype/preprocess/api.py

+    _set_MVVBS_attrs(ds_MVBS)
+    ds_MVBS["Sv"] = ds_MVBS["Sv"].assign_attrs(
+        {
+            "comment": "MVBS binned on the basis of range_sample and ping number specified as index numbers",


@leewujung pay attention to this addition. I couldn't come up with a cell_methods alternative. I was getting hung up with the specification of bins based on index numbers rather than physical units.
FYI, comment is a CF attribute.

Based on what you had for the case when MVBS is computed based on physical units (actual range in meters and ping time), how about something like below?

{ "cell_methods": ( f"ping_time: mean (interval: {ping_num} pings " "comment: ping_time is the interval start) " f"echo_range: mean (interval: {range_sample_num} samples along range " "comment: echo_range is the interval start)" ), "binning_mode": "sample number", "range_meter_interval": str(range_sample_num) + "samples along range", "ping_time_interval": str(ping_num) + "pings", }

Adding link to where I discussed CF cell_methods: #594 (review)

A challenge with your suggestion is the interval units string. Per CF:

The syntax is (interval: value unit), where value is a numerical value and unit is a string that can be recognized by UNIDATA’s Udunits package [UDUNITS].

So, using "pings" and "samples along range" would not be CF compliant. The difficulty is in trying to specify a constant interval that's based on the index number rather than the dimension units. Though CF also says this:

The unit will usually be dimensionally equivalent to the unit of the corresponding dimension, but this is not required (which allows, for example, the interval for a standard deviation calculated from points evenly spaced in distance along a parallel to be reported in units of length even if the zonal coordinate of the cells is given in degrees).

Let me chew on this.

"range_meter_interval": str(range_sample_num) + "samples along range", "ping_time_interval": str(ping_num) + "pings",

This seems confusing, since the value after "range_meter_interval" will not be in meters, nor will the value after "ping_time_interval" be in time units. It seems like the goal of these custom echopype attributes was to record the function parameters used, as is. If we find a way to settle on using cell_methods, the linkage between ping_time and echo_range and the binning method (mean) & interval values used would be explicit, so I'd then vote for retaining the original attribute names of "range_sample_num" and "ping_num".

I am not entirely understanding what you are proposing, but when proposing the cell_methods I actually forgot to check the attribute names. It should be

"range_sample_interval": str(range_sample_num) + "samples along range", "ping_interval": str(ping_num) + "pings",

ie "range_sample_interval" instead of "range_meter_interval", "ping_~~time~~_interval" instead of "ping_time_interval"

I was pointing out that these custom (echopype) attributes generated by compute_MVBS and compute_MVBS_index_binning seemed to mirror the parameters and the values fed to those functions, as is. But I realize now that wasn't true, though it was close.
I've updated the code to reflect what you have here.

echopype/preprocess/api.py

codecov-commenter · 2022-04-10T22:03:52Z

Codecov Report

Merging #615 (5a685f4) into dev (642154c) will decrease coverage by 5.70%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##              dev     #615      +/-   ##
==========================================
- Coverage   78.74%   73.04%   -5.71%     
==========================================
  Files          40       19      -21     
  Lines        3614     1165    -2449     
==========================================
- Hits         2846      851    -1995     
+ Misses        768      314     -454

Flag	Coverage Δ
unittests	`73.04% <94.87%> (-5.71%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
echopype/echodata/echodata.py	`56.50% <0.00%> (-30.78%)`	⬇️
echopype/preprocess/api.py	`70.58% <90.90%> (-19.21%)`	⬇️
echopype/calibrate/__init__.py	`100.00% <100.00%> (ø)`
echopype/calibrate/api.py	`83.78% <100.00%> (+0.45%)`	⬆️
echopype/calibrate/calibrate_azfp.py	`95.74% <100.00%> (ø)`
echopype/calibrate/calibrate_base.py	`84.94% <100.00%> (-2.16%)`	⬇️
echopype/calibrate/calibrate_ek.py	`92.77% <100.00%> (-0.73%)`	⬇️
echopype/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
echopype/echodata/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
echopype/preprocess/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
... and 36 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

…S_index_binning cell_methods and other attributes

for more information, see https://pre-commit.ci

emiliom · 2022-04-12T07:53:09Z

Pushed new commits:

Replace Sp with TS. See Add variable Sv attributes to output dataset from compute_Sv #582 (comment)
Update dB reference levels and symbology. See Add variable Sv attributes to output dataset from compute_Sv #582 (comment)
Add MVBS_index_binning cell_methods attribute and update other provenance attributes

leewujung · 2022-04-12T15:36:45Z

Great, thanks @emiliom !
The failed pre-commit check are just 2 lines that are too long:

echopype/echopype/preprocess/api.py

Line 131 in dd369c1

    
           f"ping_time: mean (interval: {ping_time_bin_resvalue} {ping_time_bin_resunit_label} "

and

echopype/echopype/preprocess/api.py

Line 197 in dd369c1

    
           "comment": "MVBS binned on the basis of range_sample and ping number specified as index numbers",

For readability an #noqa may be in order.

leewujung · 2022-04-12T16:07:54Z

echopype/calibrate/api.py

        if "water_level" in echodata.platform.data_vars.keys():
            # add water_level to the created xr.Dataset
            sv_dataset["water_level"] = echodata.platform.water_level


@emiliom : Not sure if this should be in another PR:

Looking at this now, I realized the check and addition of water_level should be in the outputs for both compute_Sv and compute_TS so that users can use this to convert echo_range to depth easily.

Also in the realm of this PR, the attributes of water_level can be set in _set_MVBS_attrs below under echopype/preprocess/api.py.

Related to #516 #583 #586

It could be a separate PR. But this looks like an easy change w/o additional ramifications that will lead to protracted discussions. It's also in line with the broader improvements to compute_Sp/TS. So, I'll just add it to this PR.

Sounds good. Thank you @emiliom !

Also in the realm of this PR, the attributes of water_level can be set in _set_MVBS_attrs below under echopype/preprocess/api.py.

Actually, water_level retains its original attributes from the echodata Platform group. So, no attribute assignment is taking place in calibrate/api.py.

I've implemented the addition of water_level to compute_TS (and in the process it made the code quite a bit simpler!). I'll push it to the PR in a bit.

emiliom · 2022-04-13T07:59:48Z

For readability an #noqa may be in order.

Done

leewujung · 2022-04-13T14:48:06Z

I think you had a typo on noqa 🤪

emiliom · 2022-04-13T17:10:56Z

I think you had a typo on noqa

😩 Sigh. I'll fix it now.

emiliom · 2022-04-13T17:56:55Z

pre-commit is now passing. Nice!

leewujung · 2022-04-13T17:58:35Z

Thanks @emiliom ! I'll add a small thing about TS applicability in the docstring later today, and then merge this.

emiliom · 2022-04-13T23:58:09Z

I think what remains an open question is the cell_methods implementation. As I wrote earlier (above):

A challenge with your suggestion is the interval units string. Per CF:

The syntax is (interval: value unit), where value is a numerical value and unit is a string that can be recognized by UNIDATA’s Udunits package [UDUNITS].

So, using "pings" and "samples along range" would not be CF compliant. The difficulty is in trying to specify a constant interval that's based on the index number rather than the dimension units.

I don't have a solution. I think we should just merge this PR (after your additions), then come back to it in the future.

leewujung · 2022-04-14T00:19:15Z

@emiliom : I think you should make a decision here since I am not very familiar with the CF conventions in general.

I will merge this after my addition of TS-related docstring.

leewujung · 2022-04-14T02:44:10Z

@emiliom : do you wanna capture this leftover dilemma in a new issue? Thanks!

emiliom · 2022-04-14T18:40:09Z

@emiliom : do you wanna capture this leftover dilemma in a new issue? Thanks

Done. #629

emiliom added 2 commits April 5, 2022 18:00

Sp attributes and simplified _compute_cal attr assignment

aa9d637

Add attributes for Sp and MVBS from index binning

3162ed9

emiliom added data conversion data format Anything about data format labels Apr 6, 2022

emiliom added this to the 0.6.0 milestone Apr 6, 2022

emiliom requested a review from leewujung April 6, 2022 07:58

[pre-commit.ci] auto fixes from pre-commit.com hooks

ae00bca

for more information, see https://pre-commit.ci

emiliom commented Apr 6, 2022

View reviewed changes

emiliom mentioned this pull request Apr 9, 2022

Tracking issue for Sv and MVBS datasets upgrade #591

Closed

8 tasks

OSOceanAcoustics deleted a comment from emiliom Apr 10, 2022

leewujung reviewed Apr 10, 2022

View reviewed changes

echopype/preprocess/api.py Outdated Show resolved Hide resolved

Correct _set_MVBS spelling error

3fd273a

emiliom and others added 3 commits April 11, 2022 14:04

Change Sp to TS, first steps

f01b6e0

Replace Sp with TS. Update dB reference levels and symbology. Add MVB…

34ce309

…S_index_binning cell_methods and other attributes

[pre-commit.ci] auto fixes from pre-commit.com hooks

dd369c1

for more information, see https://pre-commit.ci

leewujung reviewed Apr 12, 2022

View reviewed changes

emiliom added 2 commits April 13, 2022 00:54

Add water_level to compute_TS calibration

a14347d

Add #noqa to a couple of long lines

5f42087

mispelled noqa

40433d7

add TS details in docstring

5a685f4

leewujung merged commit a33edb7 into OSOceanAcoustics:dev Apr 14, 2022

emiliom mentioned this pull request Apr 14, 2022

Target strength (TS / Sp) CF cell_methods attribute challenges #629

Open

emiliom deleted the sp_mvbs_attr branch December 24, 2022 03:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add attributes for Sp and MVBS from index binning #615

Add attributes for Sp and MVBS from index binning #615

emiliom commented Apr 6, 2022

emiliom Apr 6, 2022

leewujung Apr 10, 2022

emiliom Apr 10, 2022

emiliom Apr 10, 2022 •

edited

Loading

emiliom Apr 10, 2022

leewujung Apr 11, 2022

emiliom Apr 12, 2022

codecov-commenter commented Apr 10, 2022 •

edited

Loading

emiliom commented Apr 12, 2022

leewujung commented Apr 12, 2022

leewujung Apr 12, 2022 •

edited

Loading

leewujung Apr 12, 2022

emiliom Apr 12, 2022 •

edited

Loading

leewujung Apr 12, 2022

emiliom Apr 13, 2022

emiliom commented Apr 13, 2022

leewujung commented Apr 13, 2022

emiliom commented Apr 13, 2022

emiliom commented Apr 13, 2022

leewujung commented Apr 13, 2022

emiliom commented Apr 13, 2022

leewujung commented Apr 14, 2022 •

edited

Loading

leewujung commented Apr 14, 2022

emiliom commented Apr 14, 2022

Add attributes for Sp and MVBS from index binning #615

Add attributes for Sp and MVBS from index binning #615

Conversation

emiliom commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emiliom Apr 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 10, 2022 • edited Loading

Codecov Report

emiliom commented Apr 12, 2022

leewujung commented Apr 12, 2022

leewujung Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emiliom Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emiliom commented Apr 13, 2022

leewujung commented Apr 13, 2022

emiliom commented Apr 13, 2022

emiliom commented Apr 13, 2022

leewujung commented Apr 13, 2022

emiliom commented Apr 13, 2022

leewujung commented Apr 14, 2022 • edited Loading

leewujung commented Apr 14, 2022

emiliom commented Apr 14, 2022

emiliom Apr 10, 2022 •

edited

Loading

codecov-commenter commented Apr 10, 2022 •

edited

Loading

leewujung Apr 12, 2022 •

edited

Loading

emiliom Apr 12, 2022 •

edited

Loading

leewujung commented Apr 14, 2022 •

edited

Loading