-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 numpy dtypes bug in cuda.to_gpu and cuda.copy #4380
Conversation
chainer/backends/cuda.py
Outdated
else: | ||
_integer_types = six.integer_types | ||
_integer_types = six.integer_types + (numpy.integer,) |
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.
How about factoring out common part?:
_integer_types = six.integer_types + (numpy.integer,)
if six.PY2:
_integer_types += (_newint,)
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.
That's a good idea, thanks.
chainer/backends/cuda.py
Outdated
@@ -219,7 +219,7 @@ def get_device(*args): | |||
|
|||
def _get_device(*args): | |||
for arg in args: | |||
if type(arg) in _integer_types: | |||
if isinstance(arg, _integer_types): |
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.
In this way bool
is evaluated as True
.
How about excluding bool
explicitly?
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.
Good catch, done.
chainer/backends/cuda.py
Outdated
@@ -219,7 +218,8 @@ def get_device(*args): | |||
|
|||
def _get_device(*args): | |||
for arg in args: | |||
if type(arg) in _integer_types: | |||
if isinstance(arg, _integer_types) and not numpy.issubdtype( | |||
type(arg), numpy.bool_): |
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 bool
, not numpy.bool_
.
(isinstance(numpy.bool_, numpy.integer)
and isinstance(numpy.bool_, six.integer_types)
) are both False
, but isinstance(bool, six.integer_types)
is True
).
So the correct condition would be:
if type(arg) is not bool and isinstance(arg, _integer_types):
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 my condition also correct? It works for both numpy.bool
, numpy.bool_
and bool
.
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 see. So the outcome would be the same, but it does duplicate check and slower (numpy.bool_
never satisfies the first condition). Also note that numpy.bool
and bool
are identical.
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.
Aha, bool and np.bool are identical. Then your solution is cleaner. Thanks! I will update it.
Thank you for the fix. |
LGTM! |
Fix numpy dtypes bug in cuda.to_gpu and cuda.copy
I found a bug in
cuda.to_gpu
andcuda.copy
that would not change the device when the device identifier is a numpy dtype. Users might use numpy to generate the device ids, and would therefore be subject to this subtle bug.MWE
Illustrating the problem for
to_gpu
, but the same thing happens forcopy
.Output
Without this PR:
With this PR (expected):