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

Drop artificially created ping_time dimension #1083

Merged
merged 9 commits into from
Jul 24, 2023

Conversation

leewujung
Copy link
Member

This PR addresses #1057 by removing ping_time dimension artificially added to some variables in set_groups_*.

The PR also adds comments on sections of test functions where explicit ping_time dim drop is required.

@leewujung leewujung added the data format Anything about data format label Jul 22, 2023
@leewujung leewujung added this to the 0.8.0 milestone Jul 22, 2023
@leewujung leewujung requested a review from emiliom July 22, 2023 22:13
@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2023

Codecov Report

Merging #1083 (97c33ec) into dev (9fda953) will decrease coverage by 11.64%.
The diff coverage is 81.81%.

@@             Coverage Diff             @@
##              dev    #1083       +/-   ##
===========================================
- Coverage   78.15%   66.51%   -11.64%     
===========================================
  Files          65       32       -33     
  Lines        6229     4056     -2173     
===========================================
- Hits         4868     2698     -2170     
+ Misses       1361     1358        -3     
Flag Coverage Δ
unittests 66.51% <81.81%> (-11.64%) ⬇️

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

Impacted Files Coverage Δ
echopype/calibrate/calibrate_ek.py 98.38% <ø> (ø)
echopype/convert/set_groups_azfp.py 77.38% <ø> (-20.24%) ⬇️
echopype/convert/set_groups_ek60.py 75.00% <0.00%> (-17.75%) ⬇️
echopype/convert/set_groups_ek80.py 82.52% <0.00%> (-14.87%) ⬇️
echopype/calibrate/cal_params.py 91.79% <100.00%> (-0.58%) ⬇️
echopype/calibrate/ek80_complex.py 95.45% <100.00%> (ø)
echopype/consolidate/split_beam_angle.py 78.37% <100.00%> (-10.82%) ⬇️

... 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
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.

Looks good! My comments are only about some of the comments you added.

echopype/tests/calibrate/test_cal_params_integration.py Outdated Show resolved Hide resolved
echopype/convert/set_groups_azfp.py Outdated Show resolved Hide resolved
echopype/convert/set_groups_ek60.py Outdated Show resolved Hide resolved
@leewujung
Copy link
Member Author

Sound good. I think I've addressed your comments and will merge after the tests run through (though I did not make any code changes in the new commits).

@leewujung leewujung merged commit ed79cd8 into OSOceanAcoustics:dev Jul 24, 2023
3 checks passed
lsetiawan pushed a commit to lsetiawan/echopype that referenced this pull request Jul 28, 2023
* remove artifically added ping_time dim from set_groups_*

* remove check of ping_time in split_beam_angle.py::get_angle_complex_samples

* add additional comments to test_cal_params_integration.py

* add additional comments to calibrate_ek.py::_assimilate_ecs_cal_params

* remove unused (already commented out) lines in ek80_complex.py::compress_pulse

* update cal_param.py::_get_interp_da on handling with some params now without ping_time dim

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

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

* remove isel(ping_time=0) for beam_type split_beam_angle.py::get_angle_complex_samples

* revise comments and fix typo

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@leewujung leewujung deleted the drop-ping-time-dim branch July 21, 2024 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data format Anything about data format
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants