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

Handling of non-positive values causing log10 warnings on EK calibration #986

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented Mar 15, 2023

Handle values <= 0 (0 and negative values) in calibration compute to eliminate log10 warnings. For the dataarrays causing these warnings, sets non-negative values to a slightly positive non-zero value.

Addresses #737

Note: when I ran the test suite locally there were a bunch of errors unrelated to the warnings addressed here. I'm going to assume there's some other mess in my test setup or some temporary glitch, and let the CI tests take a crack. All the log10 warnings in test_range_integration.py are gone.

@emiliom emiliom added the bug Something isn't working label Mar 15, 2023
@emiliom emiliom added this to the 0.6.4.1 milestone Mar 15, 2023
@emiliom emiliom requested a review from leewujung March 15, 2023 05:03
@emiliom
Copy link
Collaborator Author

emiliom commented Mar 15, 2023

There was just one failure in the CI, and it appears to be directly related to the change in this PR.

@leewujung
Copy link
Member

@emiliom : Thanks! I will take a look tomorrow morning!

@leewujung
Copy link
Member

leewujung commented Mar 15, 2023

I checked the CI failure and the code, and found the following:

  • for the parts involving np.log10(tvg_mod_range): the warning will appear because the first small section of tvg_mod_range is negative, and the value will be NaN
  • for the parts involving 10 * np.log10(prx): the warning will appear because the last element is 0, so the resulting value will be Inf

Let's change both so that the values are replaced with NaN before the log10 operation, since these the ranges that NaN at output does not impact practical functionality.

@leewujung leewujung modified the milestones: 0.6.4.1, 0.7.0 Mar 15, 2023
@emiliom
Copy link
Collaborator Author

emiliom commented Mar 15, 2023

Done. I've pushed a commit that replaces values with np.nan instead of 0. CI tests are churning.

@emiliom emiliom merged commit f28f609 into OSOceanAcoustics:dev Mar 15, 2023
@emiliom emiliom deleted the cal-log10warning branch March 15, 2023 21:26
@leewujung leewujung modified the milestones: 0.7.0, 0.6.4.1 Mar 16, 2023
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.

2 participants