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

Fill in NaN for missing EK80 coefficients [all tests ci] #1458

Merged
merged 8 commits into from
Feb 19, 2025

Conversation

leewujung
Copy link
Member

@leewujung leewujung commented Feb 17, 2025

This PR change EK80 set_vendor to fill in np.nan for missing filter coeffs across all channels.

Currently we have below in our test file:

This addresses #1317 and #1413.

@leewujung leewujung changed the title Sequence ek80 coeff Fill in NaN for missing EK80 coefficients Feb 17, 2025
@leewujung leewujung changed the title Fill in NaN for missing EK80 coefficients Fill in NaN for missing EK80 coefficients [all tests ci] Feb 17, 2025
@leewujung leewujung closed this Feb 17, 2025
@leewujung leewujung reopened this Feb 17, 2025
@leewujung
Copy link
Member Author

The failed tests do not appear to be related to the changes made in this PR. I'll check soon.

@ctuguinay
Copy link
Collaborator

Yeah, I tried one of the failed tests with an older scipy version and it passed, but after upgrading to the latest scipy that test failed.

@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.33%. Comparing base (9f56124) to head (a5a92f2).
Report is 186 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1458      +/-   ##
==========================================
+ Coverage   83.52%   85.33%   +1.81%     
==========================================
  Files          64       72       +8     
  Lines        5686     6579     +893     
==========================================
+ Hits         4749     5614     +865     
- Misses        937      965      +28     
Flag Coverage Δ
unittests 85.33% <100.00%> (+1.81%) ⬆️

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

ctuguinay commented Feb 17, 2025

So the conversion runs well, but when trying to calibrate the three ensemble raw file I'm getting the following error:
image
I'ma grab some lunch and investigate this.

@ctuguinay
Copy link
Collaborator

I pushed something small to cast numpy float arrays to integers when indexing.

ctuguinay
ctuguinay previously approved these changes Feb 17, 2025
Copy link
Collaborator

@ctuguinay ctuguinay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leewujung I think what you have looks good. Let me know what you think of these small changes I made.

@leewujung
Copy link
Member Author

@ctuguinay : Thanks for the changes and the added test! Great that you caught this indexing problem. I had forgotten that having nan would force the array to be float.

The only thing I have is github warns that there's no @pytest.mark.test -- I can't find it anywhere either. Was this a typo?

@ctuguinay
Copy link
Collaborator

Oh yes my bad that was for singling out a single test. Let me remove that and squash merge this. It was hidden at line 619:
image

@ctuguinay ctuguinay merged commit ead2550 into OSOceanAcoustics:main Feb 19, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
3 participants