Repository navigation
Add support for assigning values to boolean vectors in Python - #1299
Conversation
|
No actionable comments were generated in the recent review. ð âđïļ Recent review infoâïļ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ð Files selected for processing (2)
â Files skipped from review due to trivial changes (1)
ð WalkthroughWalkthroughAdded two hidden boolean-vector overloads ( Changes
Estimated code review effortðŊ 2 (Simple) | âąïļ ~10 minutes ðĨ Pre-merge checks | â 2 | â 1â Failed checks (1 warning)
â Passed checks (2 passed)
âïļ Tip: You can configure your own custom pre-merge checks in the settings. âĻ Finishing Touches𧊠Generate unit tests (beta)
ð Coding Plan
Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
|
Also, should I add a changelog entry regarding this? |
Greptile SummaryThis PR fixes a kernel codegen error that occurred when assigning to an element of a boolean vector (e.g., Changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["v[i] = value\n(bool vector)"] --> B{Dispatch lookup\nin builtins}
B -->|Before PR: no match| C["WarpCodegenError:\nmissing overload"]
B -->|After PR: match found| D{"enable_vector_component_overwrites?"}
D -->|False| E["assign_inplace\n(vector dtype=bool)"]
D -->|True| F["assign_copy\n(vector dtype=bool)"]
E --> G["vector_assign_dispatch_func\n(shared with Scalar path)"]
F --> G
G --> H{Index type}
H -->|int| I["types_equal check:\nvalue vs vec._wp_scalar_type_"]
H -->|slice_t| J["type_is_vector check +\nlength compatibility"]
I --> K["Emit C++/CUDA\nassignment"]
J --> K
Last reviewed commit: "Add support for assi..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@shi-eric it would be great if we could add this to the next patch release. This is my first contribution, so please let me know if there's anything I'm missing here. |
|
Thanks for the contribution! You've done a pretty good job of following project conventions and I appreciated the pull request description. It should be no problem to have this included in v1.12.1. One thing missing: the Here's an example of what that could look like, added to def test_bool_vec_assign_copy(test, device):
old_setting = wp.config.enable_vector_component_overwrites
try:
wp.config.enable_vector_component_overwrites = True
@wp.kernel(module="unique")
def bool_vec_assign_copy_kernel():
a = vec3bool_type(True, True, True)
b = a
b[1] = False
# a should be unchanged (copy semantics)
wp.expect_eq(a[0], True)
wp.expect_eq(a[1], True)
wp.expect_eq(a[2], True)
# b should reflect the mutation
wp.expect_eq(b[0], True)
wp.expect_eq(b[1], False)
wp.expect_eq(b[2], True)
wp.launch(bool_vec_assign_copy_kernel, 1, device=device)
wp.synchronize_device(device)
finally:
wp.config.enable_vector_component_overwrites = old_settingI verified that this test fails with a clear codegen error ( Could you also file a GitHub issue for the original bug (assigning to boolean vector components not working) and associate this PR to it? Then add a changelog entry under the |
shi-eric
left a comment
shi-eric
left a comment
There was a problem hiding this comment.
Just some minor things. As you revise your pull request it would help if you could rebase with main and keep changes limited to one commit for this fix, thanks!
| @wp.func | ||
| def fn(): | ||
| v = vec3bool_type(True, False, True) | ||
|
|
||
| v[0] = False | ||
| v[2] = False | ||
|
|
||
| wp.expect_eq(v[0], False) | ||
| wp.expect_eq(v[1], False) | ||
| wp.expect_eq(v[2], False) | ||
|
|
||
| v[-1] = True | ||
| v[-3] = True | ||
|
|
||
| wp.expect_eq(v[-1], True) | ||
| wp.expect_eq(v[-2], False) | ||
| wp.expect_eq(v[-3], True) | ||
|
|
||
| @wp.kernel(module="unique") | ||
| def kernel(): | ||
| fn() |
There was a problem hiding this comment.
Can you move these definitions to the module level and pick descriptive names?
This is a micro-optimization as defining kernels inside functions can result in some extra compilation overhead due to runtime hash changes.
Signed-off-by: Thomas B. Brunner <thomas.brunner@helsing.ai>
55bf8f9 to
ef09577
Compare
|
Thanks for the review!
Done (#1302)
I've added an entry there.
Good point. I thought this was needed for the assignment, but it's not. Should I remove this from this PR, as it is not relevant for fixing the core bug? Still, I've added a test for this in I've also rebased onto the latest main and kept the changes to a single commit. |
I think it's great to fix both issues at once since they're related. Changes look good, I'll test and merge this today. |
|
/ok to test ef09577 |
Description
Currently, attempting to write to a field of a boolean vector within a kernel will lead to the error:
This PR adds support for assigning values to boolean vectors.
Checklist
Test plan
I've added a test to the repo. The test is passing with the changes.
Bug fix
To reproduce this in the current main:
The compilation of the kernel with the int vector (
assign_int_vector) works, but the one with the boolean vector (assign_bool_vector) fails.Summary by CodeRabbit
New Features
Tests
Chores