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

Ignore nan spectrogram values #410

Merged
merged 7 commits into from
May 22, 2024
Merged

Conversation

iaraota
Copy link
Collaborator

@iaraota iaraota commented May 15, 2024

If the data is corrupted, the computed spectrograms may contain nan values. While these nan values are ignored in spectrogram plots, but they can cause issues in derived products such as the power spectral density and ratio spectrogram, where the presence of nan values leads to all values becoming nan, resulting in empty plots. This pull request addresses this issue by removing any potential nan values from the data used and saved in globalv.SPECTROGRAMS.

@iaraota iaraota requested a review from eagoetz May 15, 2024 18:50
@iaraota iaraota self-assigned this May 15, 2024
Copy link

codecov bot commented May 15, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 49.62%. Comparing base (720bd0c) to head (922827c).
Report is 10 commits behind head on master.

Files Patch % Lines
gwsumm/data/coherence.py 75.00% 1 Missing ⚠️
gwsumm/data/spectral.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
- Coverage   49.64%   49.62%   -0.03%     
==========================================
  Files          60       60              
  Lines        8823     8848      +25     
==========================================
+ Hits         4380     4390      +10     
- Misses       4443     4458      +15     
Flag Coverage Δ
Linux 49.62% <83.33%> (-0.03%) ⬇️
macOS 49.62% <83.33%> (?)
python3.10 49.62% <83.33%> (-0.03%) ⬇️
python3.11 49.62% <83.33%> (-0.03%) ⬇️
python3.9 49.62% <83.33%> (-0.03%) ⬇️

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.

@eagoetz
Copy link
Collaborator

eagoetz commented May 16, 2024

@iaraota thanks! I was wondering: if I understand correctly, this check handles the possibility of new incoming spectrogram data to be added to the global memory variable will be excluded, but what about data already in the archive? Is the point that the corrupted data would never have made it into the archive file in the first place because of this check?

@iaraota
Copy link
Collaborator Author

iaraota commented May 16, 2024

@iaraota thanks! I was wondering: if I understand correctly, this check handles the possibility of new incoming spectrogram data to be added to the global memory variable will be excluded, but what about data already in the archive? Is the point that the corrupted data would never have made it into the archive file in the first place because of this check?

This change prevents nan values from entering the archive. Although we could add a check in the archive to delete nan data, this additional step may be unnecessary since this change already addresses the issue. Instead, we could simply delete the archive of the broken pages and rerun. Do we want to implement this extra step?

@eagoetz
Copy link
Collaborator

eagoetz commented May 16, 2024

Thanks for clarifying. I think this solution is sufficient. However I'm wondering about another impact from excluding data from the global memory variable: is it possible that some channel(s) may have some nan value and thus the spectrogram shape is different than other spectrograms. Is it possible there could be some downstream consequence of that (like computing coherences or anything like that)? Are there other data elements that could be impacted by nan values?

@iaraota
Copy link
Collaborator Author

iaraota commented May 16, 2024

This function is not used in the coherence.py, so it should not be a problem in the coherence computation.

@eagoetz
Copy link
Collaborator

eagoetz commented May 16, 2024

This function is not used in the coherence.py, so it should not be a problem in the coherence computation.

Which function? I'm also wondering if other computations (like coherence) would be impacted by some channels having nan but not others. Should coherence-grams also be protected in the same way as spectrograms?

@iaraota
Copy link
Collaborator Author

iaraota commented May 16, 2024

This function is not used in the coherence.py, so it should not be a problem in the coherence computation.

Which function? I'm also wondering if other computations (like coherence) would be impacted by some channels having nan but not others. Should coherence-grams also be protected in the same way as spectrograms?

Maybe I miss understood you. We can add this check after line 288.

@iaraota
Copy link
Collaborator Author

iaraota commented May 16, 2024

@eagoetz updated the PR to also ignore nan in coherence spectrograms.

@eagoetz
Copy link
Collaborator

eagoetz commented May 16, 2024

@eagoetz updated the PR to also ignore nan in coherence spectrograms.

Awesome, thank you! One more minor suggestion: can a warning be printed so that it is more obvious when something like this happens so that it is not silently hidden from the user or log files? Thanks!

@iaraota
Copy link
Collaborator Author

iaraota commented May 20, 2024

@eagoetz updated the PR with warnings.

gwsumm/data/coherence.py Outdated Show resolved Hide resolved
gwsumm/data/spectral.py Outdated Show resolved Hide resolved
Co-authored-by: Evan Goetz <32753745+eagoetz@users.noreply.github.com>
gwsumm/data/coherence.py Outdated Show resolved Hide resolved
gwsumm/data/spectral.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@eagoetz eagoetz left a comment

Choose a reason for hiding this comment

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

LGTM

@iaraota iaraota merged commit 4ecb502 into gwpy:master May 22, 2024
9 of 10 checks passed
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.

None yet

2 participants