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

Remove min max attributes for compute_MVBS #1380

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

ctuguinay
Copy link
Collaborator

Addresses #1378

@ctuguinay ctuguinay added the enhancement This makes echopype better label Aug 23, 2024
@ctuguinay ctuguinay added this to the v0.9.1 milestone Aug 23, 2024
@ctuguinay ctuguinay self-assigned this Aug 23, 2024
@ctuguinay ctuguinay linked an issue Aug 23, 2024 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.92%. Comparing base (9f56124) to head (aed7143).
Report is 136 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1380       +/-   ##
===========================================
+ Coverage   83.52%   96.92%   +13.40%     
===========================================
  Files          64        3       -61     
  Lines        5686      195     -5491     
===========================================
- Hits         4749      189     -4560     
+ Misses        937        6      -931     
Flag Coverage Δ
unittests 96.92% <ø> (+13.40%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ctuguinay
Copy link
Collaborator Author

ctuguinay commented Aug 23, 2024

@leewujung Just removed those few lines. This should be ready for a review.

Edit: Min/Max were actually eagerly calculated twice in the MVBS operation!

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.

Oh wow, thanks for finding out the second one!

The only thing I can think of is whether not having this attribute would impact automatic plotting in xarray or holoviz. I seem to remember when I was doing the MVBS downsampling for the ship echoshader plotting, something wasn't happy when the new MVBS dataarray did not have attributes. But I don't remember which one tripped it.

I just did a simple test using the dataset on this hvplot Image example page, and it plots without problem without the actual_range attributes.

So I think we are good to merge! We should probably add a standalone separate function to add actual_range back to the MVBS dataarray. This way if it is required in some way it will be easy to produce, and can be lazily evaluated.

@ctuguinay
Copy link
Collaborator Author

@leewujung Thanks for the review! I'll add an issue for the standalone function

@ctuguinay ctuguinay merged commit d5732de into OSOceanAcoustics:main Sep 3, 2024
5 checks passed
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.

Eager computation in compute_MVBS setting of attributes
3 participants