Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: detray ranges #303

Merged
merged 41 commits into from
Oct 13, 2022
Merged

feat: detray ranges #303

merged 41 commits into from
Oct 13, 2022

Conversation

niermann999
Copy link
Contributor

@niermann999 niermann999 commented Sep 24, 2022 •
edited
Loading

Refactor iterator infrastructure and add iterators that allow chained iteration over different grid bins, as well as to pick random surfaces from the detector store instead of just ranges.

Does not do lazy evaluation though and no pipe operator

@niermann999 niermann999 added enhancement New feature or request refactor refactoring the current codes labels Sep 24, 2022
@niermann999
Copy link
Contributor Author

Still needs a lot of polishing, but at least it seems to work without killing performance. Also needs some CUDA testing

@niermann999 niermann999 force-pushed the feat-detray-ranges branch 2 times, most recently from 8fee52a to 1e64d4d Compare October 2, 2022 22:12
@beomki-yeo
Copy link
Collaborator

as well as to pick random surfaces from the detector store

Why do you pick a random surface from the detector?

beomki-yeo
beomki-yeo approved these changes Oct 4, 2022
View reviewed changes
Copy link
Collaborator

@beomki-yeo beomki-yeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I don't know well what is going on exactly in the core directory but the unit tests look solid.

core/include/detray/utils/ranges/ranges.hpp Outdated Show resolved Hide resolved
beomki-yeo
beomki-yeo reviewed Oct 4, 2022
View reviewed changes
tests/unit_tests/cuda/utils_ranges_cuda_kernel.cu

void test_single(const dindex value, dindex& check) {
dindex* result{nullptr};
cudaMallocManaged(&result, sizeof(dindex));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can transfer the single value with vecmem pointer but it's trivial

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I did not know that existed :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you maybe 'point' me there?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I tried to write a test kernel to check this out, but vecmem unique_ptr header is not compiled in device code in my PC :/... But I have seen codes that call unique_ptr in the device (acts-project/traccc#230) so there might be something wrong in my environment setup. Anyway it's not important so we can go with this

Copy link
Contributor Author

@niermann999 niermann999 Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure a unique pointer makes much sense for a single index? Or is the unique pointer not meant to do memory management in vecmem?

@beomki-yeo beomki-yeo mentioned this pull request Oct 4, 2022
Merged
@niermann999
Copy link
Contributor Author

as well as to pick random surfaces from the detector store

Why do you pick a random surface from the detector?

This was originally intended for the local navigation, because that will return surface indices that are not contiguous in the detector surface container. So I either take the enclosing subrange or pick them one by one (detray::views::pick). There might be a better solution to this though, but I left the pick view in the PR nonetheless.

@niermann999
Copy link
Contributor Author

After reimplementing some of the stl stuff, the CUDA tests seem fine now. There is some stupid code in the join view that needs some work, but that's for later. So, if the CI runs through, this can go whenever it makes sense to merge this

@niermann999 niermann999 mentioned this pull request Oct 6, 2022
Merged
@niermann999 niermann999 changed the title Feat detray ranges feat: detray ranges Oct 10, 2022
@niermann999 niermann999 force-pushed the feat-detray-ranges branch 2 times, most recently from dee83f3 to 7daa0a4 Compare October 12, 2022 11:23
@niermann999
Copy link
Contributor Author

Ok, let me merge this as discussed. This should not disturb too much of the rest of the code (I hope) and I start to need some of the type traits...

@niermann999 niermann999 merged commit 3795219 into acts-project:main Oct 13, 2022
@beomki-yeo
Copy link
Collaborator

@niermann999 Are you sure that this PR passed GitLab CI? It seems it was refused

@beomki-yeo beomki-yeo mentioned this pull request Oct 14, 2022
Merged
@beomki-yeo
Copy link
Collaborator

This was possibly a bug in thrust with older version of CUDA.
CI passes after updating the acts machine version (#314)

@niermann999
Copy link
Contributor Author

@niermann999 Are you sure that this PR passed GitLab CI? It seems it was refused

No, it refuses the pipeline for me, so I check on one of our machines myself. There (CUDA 11.8), everything was fine...

@niermann999
Copy link
Contributor Author

This was possibly a bug in thrust with older version of CUDA. CI passes after updating the acts machine version (#314)

Thank you :)

@niermann999 niermann999 mentioned this pull request Jan 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beomki-yeo beomki-yeo beomki-yeo approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request refactor refactoring the current codes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@niermann999 @beomki-yeo