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

fix: update prov attributes combine #1116

Merged
merged 4 commits into from
Aug 15, 2023

Conversation

lsetiawan
Copy link
Member

Overview

This PR address Issue #1115. Now the code checks and keep track of current attributes data from each combined files that exists. This probably can be optimized in the future, but for now it fixes the bug.

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2023

Codecov Report

Merging #1116 (0a1bc9a) into dev (cd52645) will decrease coverage by 19.98%.
Report is 3 commits behind head on dev.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              dev    #1116       +/-   ##
===========================================
- Coverage   78.27%   58.30%   -19.98%     
===========================================
  Files          65       13       -52     
  Lines        6265     1180     -5085     
===========================================
- Hits         4904      688     -4216     
+ Misses       1361      492      -869     
Flag Coverage Δ
unittests 58.30% <100.00%> (-19.98%) ⬇️

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

Files Changed Coverage Δ
echopype/echodata/combine.py 80.80% <100.00%> (+2.22%) ⬆️

... and 54 files with indirect coverage changes

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

@emiliom emiliom added this to the 0.8.0 milestone Aug 6, 2023
@emiliom emiliom added the bug Something isn't working label Aug 6, 2023
@lsetiawan lsetiawan self-assigned this Aug 8, 2023
@lsetiawan lsetiawan linked an issue Aug 14, 2023 that may be closed by this pull request
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.

The PR looks great, thanks! It does what it's supposed to do.

In test_combine_echodata_combined_append I found a fair bit of code blocks (tests) that were nearly identical except and could be consolidated to use common code. So, I did that, plus a few other smaller improvements (IMHO) in readability, in this PR to your PR: lsetiawan#1
These changes are not needed, but I think they're helpful. Once you merge my PR (hopefully there won't be any problems), feel free to merge your PR.

Simplified by using common code for repeated blocks; plus other readability tweaks
@lsetiawan
Copy link
Member Author

@emiliom please approve this PR so I can merge it. Thanks!

@lsetiawan lsetiawan merged commit 16a2b16 into OSOceanAcoustics:dev Aug 15, 2023
3 checks passed
@lsetiawan lsetiawan deleted the fix_combine branch August 15, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Bug found during combining files
4 participants