Raise ValueError if NaN found in phase_cross_correlation result #4886
Conversation
|
Hello @GenevieveBuckley! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-08-05 07:47:10 UTC |
There was a problem hiding this comment.
Thanks @GenevieveBuckley the PR is a clear improvement over the existing situation, and I think it's a reasonable trade-off with respect to performance and validity of results.
|
A possibility would be to add a note in the docstring mentioning that if don't return the error you are supposed to check that there are no nans (or to use a mask). |
The docstring already has info about the |
Description
Closes #4763
As requested by @emmanuelle in #4767 (comment)
This PR raises a
ValueErrorif a NaN value is found in the error or phase_diff, indicating that there were unexpected NaN values in the input data.Note: this check only occurs if the keyword argument
return_error=True, which is the default value for this kwarg inphase_cross_correlation. Ifreturn_error=Falsethen there is no check, and it's possible to get a garbage shift result of zero back if the input data accidentally contained NaNs. This is because it's computationally expensive to check a large array for NaN values, which is what we'd have to do ifreturn_error=False. Still, it seems helpful if we can catch at least some of these mistakes and redirect the user to the masked cross correlation functionality instead.Checklist
For reviewers
later.
__init__.py.doc/release/release_dev.rst.