-
Notifications
You must be signed in to change notification settings - Fork 76
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
Conversation
…eEK80.__init__ env param intake uses self.echodata and self.env_params
for more information, see https://pre-commit.ci
Codecov Report
📣 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
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 50 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…urces as env params, fix tests
for more information, see https://pre-commit.ci
…into fix-ek60-env
for more information, see https://pre-commit.ci
…es from EK80 data
for more information, see https://pre-commit.ci
@emiliom : I think this is ready for a review when you are available. I will add that one integration test tomorrow morning. |
…orption, add input data array test case
env_params
for EK60 calibrationenv_params
intake for calibration
@emiliom : This one is ready for a thorough review. |
There was a problem hiding this 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.
Co-authored-by: Emilio Mayorga <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…ples and _cal_complex_samples
…into fix-ek60-env
@emiliom : Thanks for reviewing this! I've made the following changes:
I had questions about two of your comments so left them open. |
Overview
This PR fixes a regression bug in v0.6.4 and ensuresenv_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
env_params.py
to address the core of Improveenv_params
intake and add test #965ed_beam_group
in_cal_power_samples
and_cal_complex_samples
get_env_params_*
functions inenv_params_integration.py
get_env_params_*
functions inenv_params.py