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

Generate triangulated mesh on a device #1077

Merged
merged 5 commits into from
May 9, 2024

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Apr 30, 2024

To avoid using any hash tables on the device, we convert the vertex representation of a icosahedron (i.e., each triangle is a set of 3 vertices) into an edge representation (each triangle is a set of 3 edges). Edge representation is much easier to do refinement on. After all refinements are done, we convert back to a vertex representation.

Master branch generates 251,658,240 triangles in ~25 seconds, while this new approach (on A100) in less than a second.

I think it is also a generally more useful algorithm to have around whenever we need it.

benchmarks/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Is using Kokkos::Arrays important? Can/should it be replaced by some user-defined type?
If we have reason to prefer array classes over potentially more descriptive user-defined types, can you say a word to why guarding for a sufficient Kokkos version is more suitable than defining our own poor man's array class in KokkosExt::? I expect we could whip up a simplified implementation in a few lines.

template <typename ExecutionSpace, typename MemorySpace>
void convertTriangles2VertexForm(
ExecutionSpace const &space,
Kokkos::View<Kokkos::Array<int, 3> *, MemorySpace> &triangles,
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we ever agree on how to pass output views when no allocation occurs. I am not a big fan of using non-const references to denote that a view is being written into. Non-const can also be interpreted as it can be resized/realloc'd or bound to some other view.

Copy link
Contributor Author

@aprokop aprokop May 1, 2024

Choose a reason for hiding this comment

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

Did we ever agree on how to pass output views when no allocation occurs.

No, we don't decided on a guideline.

I am not a big fan of using non-const references to denote that a view is being written into.

I prefer it this way, but open to arguments. My perspective is that it is much easier to understand inputs and outputs of a function from a signature. Making everything const clouds that, and one has to dig into the function to understand what view are modified.

Non-const can also be interpreted as it can be resized/realloc'd or bound to some other view.

Indeed it can.

@aprokop
Copy link
Contributor Author

aprokop commented May 1, 2024

Is using Kokkos::Arrays important? Can/should it be replaced by some user-defined type?

It can be replaced.

@aprokop
Copy link
Contributor Author

aprokop commented May 2, 2024

@dalg24 I switched to using a poor man's version of Kokkos::Array to avoid dependency on Kokkos 4.3.99. I think it is easier to use that. I don't want to use custom types as I still do v[(i+1)%3], so I don't want to use struct {a, b, c}; which would not allow index calculations.

benchmarks/triangulated_surface_distance/generator.hpp Outdated Show resolved Hide resolved
benchmarks/triangulated_surface_distance/generator.hpp Outdated Show resolved Hide resolved
benchmarks/triangulated_surface_distance/generator.hpp Outdated Show resolved Hide resolved
template <typename ExecutionSpace, typename MemorySpace>
void convertTriangles2VertexForm(
ExecutionSpace const &space,
Kokkos::View<Array<int, 3> *, MemorySpace> &triangles,
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like that your output view is now 2nd parameter followed by an input view.

Copy link
Contributor

Choose a reason for hiding this comment

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

And I would still prefer if you took the view by const reference.
You could prefix the view parameters with in_ and out_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I would still prefer if you took the view by const reference.

I still don't understand your argument for it. What is a downside for not passing by const ref?

@aprokop aprokop force-pushed the device_triangulated_refinement branch from 11248d1 to b4cb7ee Compare May 8, 2024 19:14
@aprokop
Copy link
Contributor Author

aprokop commented May 8, 2024

Rebased on master to resolve conflicts after merging #1081.

@aprokop aprokop merged commit d2f80a5 into arborx:master May 9, 2024
2 checks passed
@aprokop aprokop deleted the device_triangulated_refinement branch May 9, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants