search

OSOceanAcoustics / echopype

Enabling interoperability and scalability in ocean sonar data analysis
https://echopype.readthedocs.io/
Apache License 2.0
89 stars 70 forks source link

Optimize get vend cal params power #1285

Closed anujsinha3 closed 3 months ago

anujsinha3 commented 3 months ago
codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/OSOceanAcoustics/echopype/pull/1285/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OSOceanAcoustics) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/OSOceanAcoustics/echopype/pull/1285/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OSOceanAcoustics) | `89.03% <100.00%> (+5.23%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OSOceanAcoustics#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lsetiawan commented 3 months ago

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

anujsinha3 commented 3 months ago

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 commented 3 months ago

@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?

anujsinha3 commented 3 months ago

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 commented 3 months ago

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

leewujung commented 3 months ago

Alright, I'll merge this now.