-
Notifications
You must be signed in to change notification settings - Fork 30
Method _usm_type of Memory class should accept sycl_context #50
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
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.
Looks good for me. It would be better if there was a test.
@oleksandr-pavlyk could you please also add a test for this method?
Tests for USM: dppl/tests/dppl_tests/test_sycl_usm.py
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.
Suggested rename a variable. In other places LGTM.
ctx = <SyclContext>(context) | ||
kind = DPPLUSM_GetPointerType(self.memory_ptr, | ||
ctx.get_context_ref()) | ||
elif isinstance(context, SyclQueue): |
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.
What if name the variable context_or_queue
?
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.
Yes, makes sense. Since the argument can be either context or queue. If we do not like long names, let us just call it syclobj
or something other than context.
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.
Minor formatting nitpick aside LGTM. Thanks!
@@ -46,16 +46,30 @@ def test_memory_without_context (self): | |||
def test_memory_cpu_context (self): | |||
mobj = self._create_memory() | |||
|
|||
# CPU context | |||
# CPU context |
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.
superfluous white space
@PokhodenkoSA @diptorupd