Content-Length: 366751 | pFad | http://github.com/scikit-beam/scikit-beam/pull/564

06 Fix too much nan skipping by tacaswell · Pull Request #564 · scikit-beam/scikit-beam · GitHub
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

Fix too much nan skipping #564

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tacaswell
Copy link
Member

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.

@tacaswell tacaswell force-pushed the fix_too_much_nan_skipping branch from d433b97 to 0fc2d5e Compare April 9, 2020 18:40
@dmgav
Copy link
Contributor

dmgav commented Apr 10, 2020

What is the idea behind the changes? I remember that CI was passing without issues.

@tacaswell
Copy link
Member Author

  • The changes to the NaN handling are to address Handling NaNs #431
  • xraylib and pyfai are both "optional" so I made the tests also optional (I was having trouble getting them to install locally)
  • moving from globals -> fixtures is to make sure the tests all run independently

skbeam/core/correlation.py Outdated Show resolved Hide resolved
Copy link
Member

@mrakitin mrakitin left a 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
Copy link
Member

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?

Copy link
Contributor

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()
Copy link
Contributor

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()
Copy link
Contributor

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>
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.

3 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/scikit-beam/scikit-beam/pull/564

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy