-
Notifications
You must be signed in to change notification settings - Fork 249
Add type annotations to algorithms #895
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
base: main
Are you sure you want to change the base?
Conversation
4803d16 to
6ba057f
Compare
6ba057f to
ba5f344
Compare
| # FIXME: other scan_dtypes below use uint? | ||
| if len(ary) > np.iinfo(np.int32).max: | ||
| scan_dtype = np.int64 | ||
| scan_dtype = np.dtype(np.int64) | ||
| else: | ||
| scan_dtype = np.int32 | ||
| scan_dtype = np.dtype(np.int32) |
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.
Not quite sure if this was on purposes, so wanted to bring it up?
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 used to be pretty sloppy about "dtype" vs "type". Not opposed to cleaning that up.
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.
Ah, I meant that this is using np.int and the other places are using np.uint?
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.
Oh. For copy_if, I think uint will also be OK.
| index_dtype: DTypeLike = np.int32, | ||
| key_dtype: DTypeLike = np.uint32, |
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.
Are these dtypes always meant to be integers? I'm guessing yes because "radix sort". (need to update key_dtype attribute if yes)
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.
Yep.
pyopencl/algorithm.py
Outdated
| double_support=all( | ||
| has_double_support(dev) for dev in self.context.devices), |
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.
Should this be using self.devices instead of self.context.devices? Not quite sure what the use case is for setting self.devices to something different though..
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.
self.devices is better.
| from pytools.persistent_dict import WriteOncePersistentDict | ||
|
|
||
| import pyopencl as cl | ||
| import pyopencl._mymako as mako |
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.
Was this on purpose? From a quick grep, this seems to be the only place that uses _mymako.
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.
Yeah. Everywhere else should switch to _mymako, too. Mako isn't a hard dependency, and all _mymako does is provide a nicer error message. For type checking, you could go
if TYPE_CHECKING:
import mako
else:
import pyopencl._mymako as mako| if neutral is None: | ||
| from warnings import warn | ||
| warn("not specifying 'neutral' is deprecated and will lead to " | ||
| "wrong results if your scan is not in-place or your " | ||
| "'output_statement' does something otherwise non-trivial", | ||
| stacklevel=2) | ||
| raise ValueError("must provide a 'neutral' element in scan") |
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.
Setting this to None would raise an exception below in _process_code_for_macro(neutral), so it doesn't seem like it's supported at 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.
Sounds good, kill it.
No description provided.