-
Notifications
You must be signed in to change notification settings - Fork 12
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
Bug fixes to snr module and related C++ code #598
Conversation
@wangyinz looks like this runs clean. Assuming black didn't do something weird the tests should again pass. I would urge you to quickly review what I did and merge this branch once you have a feel for what I did. |
@wangyinz one other detail. I intentionally left some debug scaffolding in snr.py. It is commented out here. I might need to quickly resurrect the debugging stuff if I something else goes wrong so don't worry about that for now. Help me remember to clear it out, however, when I finish this current round of testing with a full up extended usarray data set. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #598 +/- ##
==========================================
+ Coverage 58.66% 58.70% +0.03%
==========================================
Files 156 156
Lines 25392 25419 +27
Branches 1518 1519 +1
==========================================
+ Hits 14897 14921 +24
- Misses 10028 10031 +3
Partials 467 467 ☔ View full report in Codecov by Sentry. |
I don't know why codecov complained on this last push and not the one before is a mystery. snr.py actually needs a lot of work to improve the pytest module and these are picky compared to the bigger sections not being currently covered. @wangyinz let me know how you want to resolve this. |
This branch implements bug fixes I found in more extensive testing of the snr module function broadband_snr_QC with more marginal signal to noise ratio data. Testing uncovered a few problems this branch fixes:
I haven't run testing on this because the changes to the C++ code could conceivably have broader impacts, but I don't really think so. I'm going to see if the github push works here. If it does we should merge this commit after I also do some tidying of snr.py with black.