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.

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

Standardize backscatter_r/i long_name, and correct units #1047

Merged
merged 3 commits into from
May 24, 2023

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented May 22, 2023

Addresses #643.

  • Standard long_name attribute in backscatter_r and backscatter_i variables for all instruments (except ad2cp) to use the SONAR-netCDF v1 strings. Now being set in single place, in the long_name attribute strings in the echodata/convention/1.0.yml file.
  • Set AZFP backscatter_r units string to "count", per recent discussion with @leewujung and in "Processed" power data in EK60/80 backscatter_r #643
  • For EK80, correct wrong unit backscatter_r / backscatter_i assignment being made in some cases, where the unit was set to "V" rather than "dB". @leewujung please double check this!

Note: I used # noqa for a couple of long lines, but black apparently ignored it. I guess we should remove the # noqa flags?


Side note: in my local test runs, I had two failures:

FAILED echopype/tests/calibrate/test_cal_params.py::test_get_cal_params_EK80_BB[in_da_freq_dep_with_scaling] (line 549)
  - AssertionError: assert False

FAILED echopype/tests/utils/test_utils_log.py::test_init_logger (line 25)
  - AssertionError: assert 4 == 2

I've had these failures happen sporadically since March! I don't know why.

@emiliom emiliom added bug Something isn't working data format Anything about data format labels May 22, 2023
@emiliom emiliom added this to the 0.7.2 milestone May 22, 2023
@emiliom emiliom self-assigned this May 22, 2023
@emiliom emiliom requested a review from leewujung May 22, 2023 21:27
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2023

Codecov Report

Merging #1047 (9237e79) into dev (a09d8f3) will decrease coverage by 2.94%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##              dev    #1047      +/-   ##
==========================================
- Coverage   80.79%   77.85%   -2.94%     
==========================================
  Files          67       18      -49     
  Lines        6086     2908    -3178     
==========================================
- Hits         4917     2264    -2653     
+ Misses       1169      644     -525     
Flag Coverage Δ
unittests 77.85% <ø> (-2.94%) ⬇️

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

Impacted Files Coverage Δ
echopype/convert/set_groups_azfp.py 97.46% <ø> (ø)
echopype/convert/set_groups_base.py 91.97% <ø> (-1.46%) ⬇️
echopype/convert/set_groups_ek60.py 92.56% <ø> (ø)
echopype/convert/set_groups_ek80.py 97.02% <ø> (ø)

... and 50 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

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

Thanks @emiliom : the changes look good. The "noqa" stuff is weird. I wonder if it does not work after an "," since all other working cases are not preceded by a comma.

One thing I saw while going through this is the messe organization of parsed2zarr code sprinkled in set_groups_base.py. This is something hopefully #966 will address.

Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

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

Thanks @emiliom : the changes look good. The "noqa" stuff is weird. I wonder if it does not work after an "," since all other working cases are not preceded by a comma.

One thing I saw while going through this is the messe organization of parsed2zarr code sprinkled in set_groups_base.py. This is something hopefully #966 will address.

Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

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

Thanks @emiliom : the changes look good. The "noqa" stuff is weird. I wonder if it does not work after an "," since all other working cases are not preceded by a comma.

One thing I saw while going through this is the messe organization of parsed2zarr code sprinkled in set_groups_base.py. This is something hopefully #966 will address (I've added this to the issue description TODOs).

@emiliom
Copy link
Collaborator Author

emiliom commented May 23, 2023

Thanks @emiliom : the changes look good.

Great!

The "noqa" stuff is weird. I wonder if it does not work after an "," since all other working cases are not preceded by a comma.

Good suggestion. I may give it a try.

One thing I saw while going through this is the messe organization of parsed2zarr code sprinkled in set_groups_base.py. This is something hopefully #966 will address (I've added this to the issue description TODOs).

Yeah, there was effectively a duplication of code. I didn't try to clean it up. As you said, hopefully some it will be addressed in #966. Thanks for flagging it in that issue's TODOs.

You approved this PR, so it sounds like I can merge it.

@leewujung
Copy link
Member

it sounds like I can merge it
Yes, if you could either remove the #noqa or fix it, that'll be good ;)

@emiliom
Copy link
Collaborator Author

emiliom commented May 24, 2023

I've removed the # noqa flags that are being ignored. BTW, while doing that I saw other # noqa flags in one of these modules that follow lines that end in a comma. So, there must be something else going on. Oh well.

@emiliom emiliom merged commit d08f1f6 into OSOceanAcoustics:dev May 24, 2023
@emiliom emiliom deleted the backscatter_vars branch May 24, 2023 16:44
@leewujung leewujung modified the milestones: 0.7.2, 0.8.0 Jul 15, 2023
lsetiawan pushed a commit to lsetiawan/echopype that referenced this pull request Jul 28, 2023
…stics#1047)

* Define backscatter_r and backscatter_i long_name in the convention yaml file, then use it consistently everywhere

* Correct backscatter_r and backscatter_i units for AZFP (count, not dB) and EK80 (dB, not V in some cases)

* Remove some non-operational # noqa flags added recently to set_groups_ modules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working data format Anything about data format
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants