-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
REF: avoid getattr pattern for diff_2d; use fused types #29120
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
Conversation
|
||
for name, c_type, dest_type, in dtypes: | ||
yield name, c_type, dest_type | ||
ctypedef fused diff_t: |
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.
isn’t this numeric? can we consolidate these later
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 hope so
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.
lgtm
|
||
# Disable for unsupported dtype combinations, | ||
# see https://github.com/cython/cython/issues/2646 | ||
if out_t is float32_t: |
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.
assume this a later followup
thanks |
raise NotImplementedError | ||
else: | ||
if (diff_t is float32_t or diff_t is int8_t or diff_t is int16_t): | ||
raise NotImplementedError |
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.
@WillAyd you may have noticed a bunch of build warnings about unreachable code have started showing up. I'm pretty sure this is the reason. Any thoughts on more graceful ways to get the combination of dtypes we're interested in? (see the GH link on L27)
The same dtype-combination thing is the only reason why algos_take_helper isn't converted to fused types; it'd be really nice to clear 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.
Don't know off the top of my head - maybe an Enum using the type as a member?
No description provided.