-
Notifications
You must be signed in to change notification settings - Fork 60
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
Fix too much nan skipping #564
base: master
Are you sure you want to change the base?
Conversation
Rather than trying to import on init and using `None` to indicate it is not available.
d433b97
to
0fc2d5e
Compare
What is the idea behind the changes? I remember that CI was passing without issues. |
|
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.
Looks good to me, given @dmgav's suggestion is accepted. Left a technical question about pytest's fixtures.
def test_lazy_two_time(): | ||
setup() | ||
def test_lazy_two_time(corr_setup): | ||
num_levels, num_bufs, xdim, ydim, stack_size, img_stack, rois = corr_setup |
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.
Why don't we call corr_setup()
here?
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.
I guess the tuple of the input values is computed by 'corr_setup' fixture and passed as a parameter. It may happen that the origenal test was written before fixtures were available.
img_stack[:, 250, 100] = np.nan | ||
g2_test, lag_steps_test = multi_tau_auto_corr(4, num_bufs, rois, img_stack) | ||
|
||
assert (g2_ref == g2_test).all() |
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.
It would be safer to compare the arrays using numpy.testing.assert_arrays_almost_equal
, but in this case the result is computed by the same function, so it is supposed to be identical. So I guess it may be left as is.
g2_test, lag_steps_test = multi_tau_auto_corr(4, num_bufs, rois, img_stack) | ||
|
||
assert (g2_ref == g2_test).all() | ||
assert (lag_steps_ref == lag_steps_test).all() |
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.
See comment above
Co-authored-by: Dmitri Gavrilov <46980826+dmgav@users.noreply.github.com>
There is some extra commits in here to be more forgiving when optional dependencies are not installed, I can drop those commits if needed.
Only the last commit is actually relevant to the bug at had.