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.

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

Update DefaultIndexableGetter to work with all valid geometries #1042

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Mar 14, 2024

Right now, DefaultIndexableGetter only passes through points and boxes, unnecessarily restricting things. One, for example, cannot do
ArborX::BVH<MemorySpace, Triangle> bvh(space, Kokkos::View<Triangle*>);.

This patch extends passed through geometries to all that are considered valid in GeometryTraits.

@aprokop aprokop added the bug Something isn't working label Mar 14, 2024
@dalg24
Copy link
Contributor

dalg24 commented Mar 14, 2024

Any example test to demonstrate that works with this patch?

@dalg24
Copy link
Contributor

dalg24 commented Mar 14, 2024

Also why did you classify this as a bug?

@aprokop
Copy link
Contributor Author

aprokop commented Mar 14, 2024

Any example test to demonstrate that works with this patch?

We currently don't. The triangular example change in #936 still uses PairValueIndex.
I plan to introduce a distance to a triangular wall example in the near future that will exercise it.

Also why did you classify this as a bug?

Because it is impossible to construct BVH by passing all ArborX-native geometries. You can do
BVH<MemorySpace, Box> bvh(space, boxes);
but not
BVH<MemorySpace, Triangle> bvh(space, triangles);
This seems like unexpected behavior to me.

@aprokop aprokop merged commit 40ca8ba into arborx:master Mar 14, 2024
1 of 2 checks passed
@aprokop aprokop deleted the update_default_indexable_getter branch March 14, 2024 23:07
@dalg24
Copy link
Contributor

dalg24 commented Mar 14, 2024

Is that a regression? If not how is it a bug?
Also tests pass before and after (actually they don't and you didn't say why it is ok to merge with some build failing) your PR so how do we avoid regressions.
What was the rush to merge?

@aprokop
Copy link
Contributor Author

aprokop commented Mar 14, 2024

It is not a regression in the usual sense. It is a bug because what i expected to work doesn't. You can say we didn't document that it does and that it's an enhancement then, but I did expect this to work, and was surprised that it didn't. So it's a bug in my eyes.

The builds passed. The only failures are the couple warnings from the build containers. So, i didn't feel there was much to document.

I merged it so that I could introduce the fix for temporaries. That PR would have conflicts with this one, and i felt this one was in a good enough state. I admit I rushed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants