-
Notifications
You must be signed in to change notification settings - Fork 1
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
NaNs were messing up phase_change
#98
base: main
Are you sure you want to change the base?
Conversation
merge #97 before merging this PR |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
=======================================
Coverage 95.62% 95.63%
=======================================
Files 15 15
Lines 938 940 +2
=======================================
+ Hits 897 899 +2
Misses 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -176,7 +182,10 @@ def _iteratively_denoise_sf_amplitudes( | |||
|
|||
# project onto the native amplitudes to obtain an "updated_derivative" | |||
# Fh' = (D_F' + F) * [|Fh| / |D_F' + F|] | |||
denoised_difference_sfs, native = filter_common_indices(denoised_difference_sfs, native) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you either
- filter outside the while loop? or
- calculate the indices used for filtration outside the loop?
I'm sure it doesn't affect the code's correctness, but it unnecessarily costly as written. Not a big deal if it's a hassle.
Using
meteor.phaseboost
, it was possible to getNaN
s in the output metadata:During iterative TV, it's possible that we compare arrays of native/derivative data without the same indices. In this case,
NaN
s are generated at the positions of mismatched HKL indices, but dealt with gracefully in downstream code... except for theaverage_phase_diff_in_degrees
function in utils.This PR updates that function to simply ignoreBased on feedback from @kmdalton, we proactively filter out non-common indices, which precludes the generation of the offendingNaN
values.NaN
s in the first place.A regression test added to enforce this.