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

Switch spheres to new hyper-dimensional ones #1146

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Sep 17, 2024

No description provided.

@aprokop aprokop added the backward incompatible Modifications that may break users' code label Sep 17, 2024
@@ -55,7 +55,7 @@ struct ArborX::AccessTraits<Spheres, ArborX::PredicatesTag>
static KOKKOS_FUNCTION auto get(Spheres const &d, std::size_t i)
{
return ArborX::intersects(
ArborX::Sphere{{{d.d_x[i], d.d_y[i], d.d_z[i]}}, d.d_r[i]});
ArborX::Sphere{ArborX::Point{d.d_x[i], d.d_y[i], d.d_z[i]}, d.d_r[i]});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Much nicer!

@@ -110,8 +110,7 @@ struct approx_expand_by_radius<PointTag, Point>
{
constexpr int DIM = GeometryTraits::dimension_v<Box>;
using Coordinate = GeometryTraits::coordinate_type_t<Point>;
return ExperimentalHyperGeometry::Sphere<DIM, Coordinate>{
Kokkos::bit_cast<::ArborX::Point<DIM, Coordinate>>(point), r};
return Sphere{Kokkos::bit_cast<::ArborX::Point<DIM, Coordinate>>(point), r};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the bit_cast still necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This geometries may come from users. We do an implicit undocumented cast here as we don't have a well defined concept of a point. It's ok for now but will need to change in the future.

Comment on lines 40 to 41
auto const &hyper_point =
reinterpret_cast<::ArborX::Point<dim, Coordinate> const &>(pair.value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you still need the reinterpret_cast?

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 be replaced by bit_cast but the issue is same as above: user data.

Comment on lines 174 to 175
auto const &hyper_point =
reinterpret_cast<::ArborX::Point<dim, Coordinate> const &>(point);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User data.

Comment on lines 66 to 67
auto const &hyper_point =
reinterpret_cast<::ArborX::Point<dim> const &>(pair.value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Comment on lines 119 to 120
auto const &hyper_point =
reinterpret_cast<::ArborX::Point<dim> const &>(point);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

@aprokop
Copy link
Contributor Author

aprokop commented Sep 19, 2024

So, all the comments are about reinterpret_cast/bit_cast. The issues is that our algorithms require running algorithms or constructing ArborX geometry types from some other types. Often, these other types are provided by a user, and we convert them to something that internally we can digest. We could follow boost and explicitly construct internal data by using getters.

I think this is a larger discussion and is outside of the scope of this PR.

@aprokop
Copy link
Contributor Author

aprokop commented Sep 20, 2024

@masterleinad I have a patch to address the conversion routines. Will do a separate PR after this one is merged.

@aprokop aprokop merged commit bbd63ae into arborx:master Sep 20, 2024
2 checks passed
@aprokop aprokop deleted the arborx-2.0-spheres branch September 20, 2024 13:52
This was referenced Sep 20, 2024
@aprokop aprokop added the refactoring Code reorganization label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward incompatible Modifications that may break users' code refactoring Code reorganization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants