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

Require PyOpenCL context for HI of LazyEBSD #615

Merged
merged 9 commits into from Feb 20, 2023
Merged

Conversation

hakonanes
Copy link
Member

@hakonanes hakonanes commented Feb 16, 2023

Description of the change

This PR should fix #614 by requiring PyOpenCL and a compatible driver context to be available when trying to perform Hough indexing of a Dask array with PyEBSDIndex. By doing this check early, we ensure that PyEBSDIndex can use PyOpenCL and that it will not fall back to its CPU implementation, which then calls Numba and errors.

Will make a v0.8.1 release as soon as this PR is merged.

Progress of the PR

Minimal example of the bug fix or new feature

Error message that should be raised when PyOpenCL is installed but no compatible driver is available (error message copy/pasted):

>>> import pyopencl as cl
>>> cl.get_platforms()
[]
>>> import kikuchipy as kp
>>> s = kp.data.nickel_ebsd_small()
>>> s.hough_indexing(None, None)
UserWarning("Hough indexing of lazy signals must use PyOpenCL and a compatible driver. 
    See https://documen.tician.de/pyopencl/misc.html for details")

For reviewers

  • The PR title is short, concise, and will make sense 1 year later.
  • New functions are imported in corresponding __init__.py.
  • New features, API changes, and deprecations are mentioned in the unreleased
    section in CHANGELOG.rst.
  • New contributors are added to release.py, .zenodo.json and
    .all-contributorsrc with the table regenerated.

@hakonanes hakonanes added bug Something isn't working tests This relates to the tests labels Feb 16, 2023
@hakonanes hakonanes added this to the v0.8.1 milestone Feb 16, 2023
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
@hakonanes
Copy link
Member Author

hakonanes commented Feb 16, 2023

@ericpre: I would be very greatful if you could give your opinion on whether the following fixes #614:

https://github.com/pyxem/kikuchipy/pull/615/files#diff-3bd901a3b67244c36540d9734e61606033674b7e3ee2d92761ac14d6a6937aebR45-R58

Relevant part of PyEBSDIndex' PyOpenCL context setup: https://github.com/USNavalResearchLaboratory/PyEBSDIndex/blob/main/pyebsdindex/opencl/openclparam.py#L67.

@hakonanes hakonanes linked an issue Feb 16, 2023 that may be closed by this pull request
@hakonanes hakonanes changed the title Require PyOpenCL compatible driver for HI of LazyEBSD Require PyOpenCL context for HI of LazyEBSD Feb 16, 2023
Copy link
Contributor

@ericpre ericpre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericpre: I would be very greatful if you could give your opinion on whether the following fixes #614

I have left some comments but I don't have more time than this. One way to check is to push a branch of the https://github.com/hyperspy/hyperspy-extensions-list repo on github using this branch of kikuchipy and removing the corresponding tests being skipped.

from pyebsdindex.opencl.openclparam import OpenClParam

clparams = OpenClParam()
clparams.get_context()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use pyopencl directly instead, because the return of clparams.get_context() could be regarded as an implementation details of pyebsdindex.
I would expect that pyopencl have a clean approach to do this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I've done this instead.


clparams = OpenClParam()
clparams.get_context()
assert clparams.ctx is not None # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off the top of my head: using assert is intended for development and testing and is not a good pattern to catch runtime errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've replaced it with an if/else.

Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
@hakonanes
Copy link
Member Author

One way to check is to push a branch of the https://github.com/hyperspy/hyperspy-extensions-list repo on github

Thanks for the tip, the kikuchipy test suite passes in the "dev" runs when this PR's branch is used: https://github.com/hakonanes/hyperspy-extensions-list/actions/runs/4204973562/jobs/7296399979

Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Signed-off-by: Håkon Wiik Ånes <hwaanes@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tests This relates to the tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration test suite failure
2 participants