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

Optimize get vend cal params power #1285

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Conversation

anujsinha3
Copy link
Contributor

  • Refactor function to avoid expand_dims and sortby if required

  • test: increase co-ordinates to 200 for ping_time dimension

* Refactor function to avoid expand_dims and sortby if required

* test: increase co-ordinates to 1000 for ping_time dimension

* style: upper-casing variables

* style: upper-casing variables

* style: revert

---------

Co-authored-by: Anant Mittal <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.03%. Comparing base (6dc196a) to head (d3328fb).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1285      +/-   ##
==========================================
+ Coverage   83.79%   89.03%   +5.23%     
==========================================
  Files          64       11      -53     
  Lines        5703      976    -4727     
==========================================
- Hits         4779      869    -3910     
+ Misses        924      107     -817     
Flag Coverage Δ
unittests 89.03% <100.00%> (+5.23%) ⬆️

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.

@lsetiawan
Copy link
Member

@leewujung could you do a review on this, it looks good to me, just wanted to make sure that it's all good to you as well. Thanks! The test data is now bigger.

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.

@anujsinha3 : Thanks for the changes! I like how you avoided the expansion. Expanding the test data dimension is also great.

I suggested a small change to DATA to just make things more randomized, since the functions are supposed to be flexible to deal with arbitrary numbers. See what you think.

@anujsinha3
Copy link
Contributor Author

Hi @leewujung,
Initially, I had used the np.random.rand() as you suggested, but that caused the test assertions to fail as the test outputs also got randomized (especially for interpolation test cases). Hence I switched to np.ones().

Happy to adapt further suggestions.

@leewujung
Copy link
Member

@anujsinha3 : Locally the tests I suggested worked. Could you point to where the interpolation and failure was? It was hard for me to tell which parts actually got changed due to the reformatting. Even if the values are random, wouldn't it be possible to put the expected interpolated results based on DATA into the assertion?

* test: refactor test data

* test: refactor test data
@anujsinha3
Copy link
Contributor Author

Ah, I see the error now. I was multiplying the np.random.rand() with a constant, which caused the test case failures. I have fixed that now. Apologies for the confusion.

@leewujung
Copy link
Member

@anujsinha3 : No problem! Feel free to merge this then!

@leewujung
Copy link
Member

Alright, I'll merge this now.

@leewujung leewujung merged commit 8600a59 into OSOceanAcoustics:dev Mar 26, 2024
5 checks passed
@lsetiawan lsetiawan linked an issue Mar 27, 2024 that may be closed by this pull request
leewujung added a commit that referenced this pull request Apr 8, 2024
* Bump actions/cache from 3.0.11 to 3.2.0 (#18)

Bumps [actions/cache](https://github.com/actions/cache) from 3.0.11 to 3.2.0.
- [Release notes](https://github.com/actions/cache/releases)
- [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md)
- [Commits](actions/cache@v3.0.11...v3.2.0)

---
updated-dependencies:
- dependency-name: actions/cache
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update compress_pulse to Parallelize Convolution (#1208)

Update compress_pulse to Parallelize Convolution. 
This addresses #1164.

* Optimize `harmonize_env_param_time` (#1235)

* Remove extra elses

* Ensure harmonize_env_param_time works with dask arrays

* Remove unused import

* Optimize Frequency Differencing with Dask (#1198)

* Update frequency_differencing method: add dask optimizations if Sv data is dask array

* Update test_frequency_differencing function and incorporate test cases for dask

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

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

* Fix ci error

* Update echopype/mask/api.py

Co-authored-by: Don Setiawan <[email protected]>

* Update echopype/mask/api.py: assign attrs out of if-else

* Update echopype/mask/api.py: remove unused code

* refactor: Update dask chunks iter to use xr.map_blocks

* refactor: Cleanup code and make more explicit

* Add better comments

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

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

* Fix pre-commit ci errors

* refactor: Update algorithm to use map_blocks

Update the looping algorithm to use dask array
map_block instead. This would remove direct slicing
ambiguity and prepare for future xarray
map_block usage.

* refactor: Modify to have no hardcoded channel slicing

* refactor: Assign Sv data array

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Don Setiawan <[email protected]>

* Optimize get vend cal params power (#1285)

* Optimize get vend cal params power (#2)

* Refactor function to avoid expand_dims and sortby if required

* test: increase co-ordinates to 1000 for ping_time dimension

* style: upper-casing variables

* style: upper-casing variables

* style: revert

---------

Co-authored-by: Anant Mittal <[email protected]>

* test: update test_get_interp_da() to test on 200 time_coordinates (#3)

* test: refactor test data (#4)

* test: refactor test data (#5)

* test: refactor test data

* test: refactor test data

---------

Co-authored-by: Anant Mittal <[email protected]>

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Anant Mittal <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Don Setiawan <[email protected]>
Co-authored-by: anujsinha3 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve potential bottlenecks in compute_Sv
4 participants