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

Ensure env_params intake for calibration #985

Merged
merged 35 commits into from
Mar 18, 2023

Conversation

leewujung
Copy link
Member

@leewujung leewujung commented Mar 14, 2023

Overview

This PR fixes a regression bug in v0.6.4 and ensures env_params are taken up correctly for EK60 echosounder.

It turned out that the bug was not an omission, but that the env_params used for EK60 cal in v0.6.4 was not passed in from the correctly sanitized ones from the parent class method. This is now in a PR with minimal scope #987.

Therefore, I am putting in work in this PR to address #965 more comprehensively.

Details

  • Overhaul functions in env_params.py to address the core of Improve env_params intake and add test #965
  • Remove redundancy in passing in extra ed_beam_group in _cal_power_samples and _cal_complex_samples
  • Create integration tests for the get_env_params_* functions in env_params_integration.py
    • These verify that user-provided params are taken in to the calibration objects and stored in the final Sv output correctly
  • Create unit tests for the get_env_params_* functions in env_params.py

@leewujung leewujung added this to the 0.6.4.1 milestone Mar 14, 2023
@leewujung leewujung requested a review from emiliom March 14, 2023 19:44
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2023

Codecov Report

Merging #985 (ad0551c) into dev (8aca475) will increase coverage by 6.17%.
The diff coverage is 90.47%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##              dev     #985      +/-   ##
==========================================
+ Coverage   79.64%   85.82%   +6.17%     
==========================================
  Files          62       19      -43     
  Lines        5680     1199    -4481     
==========================================
- Hits         4524     1029    -3495     
+ Misses       1156      170     -986     
Flag Coverage Δ
unittests 85.82% <90.47%> (+6.17%) ⬆️

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

Impacted Files Coverage Δ
echopype/calibrate/cal_params.py 92.30% <ø> (ø)
echopype/calibrate/env_params.py 89.10% <87.17%> (+0.58%) ⬆️
echopype/calibrate/calibrate_azfp.py 97.29% <100.00%> (ø)
echopype/calibrate/calibrate_ek.py 98.68% <100.00%> (+<0.01%) ⬆️
echopype/calibrate/range.py 86.15% <100.00%> (ø)
echopype/consolidate/api.py 72.60% <100.00%> (-19.18%) ⬇️

... and 50 files with indirect coverage changes

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

@leewujung
Copy link
Member Author

@emiliom : I think this is ready for a review when you are available. I will add that one integration test tomorrow morning.

@leewujung leewujung changed the title Fix bug to ensure env_params for EK60 calibration Ensure env_params intake for calibration Mar 15, 2023
@leewujung leewujung added the enhancement This makes echopype better label Mar 16, 2023
@leewujung
Copy link
Member Author

@emiliom : This one is ready for a thorough review.

@leewujung leewujung modified the milestones: 0.6.4.1, 0.7.0 Mar 16, 2023
@leewujung leewujung self-assigned this Mar 17, 2023
Copy link
Collaborator

@emiliom emiliom left a comment

Choose a reason for hiding this comment

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

I wasn't thorough, but I think it looks good. I made a bunch of comments, most of them stylistic.

There's just one where I think you're missing something important, an inistance(). Take a look.

@leewujung
Copy link
Member Author

leewujung commented Mar 18, 2023

@emiliom : Thanks for reviewing this! I've made the following changes:

  • made all but one of the suggested changes (that one was incorrect even though the pattern looks similar, so I reverted)
  • added missed test cases for test_sanitize_user_env_dict
  • remove redundancy in passing in ed_beam_group in _cal_power_samples and _cal_complex_samples

I had questions about two of your comments so left them open.

@leewujung leewujung merged commit edc1d15 into OSOceanAcoustics:dev Mar 18, 2023
@leewujung leewujung deleted the fix-ek60-env branch July 21, 2024 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This makes echopype better
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants