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

Generalize GetNeighborList for points #1024

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Jan 11, 2024

  • Make it work with any points
  • Build the underlying BVH on points instead of boxes for performance
  • Add (memory space, execution space) checks

@aprokop aprokop added the performance Something is slower than it should be label Jan 11, 2024
@dalg24
Copy link
Contributor

dalg24 commented Jan 12, 2024

Although I don't see any issue with that change, you omitted to say that your PR now requires points as input which was not the case before.


auto const &hyper_point = reinterpret_cast<
ExperimentalHyperGeometry::Point<dim, Coordinate> const &>(point);
return intersects(ExperimentalHyperGeometry::Sphere<dim, Coordinate>{
Copy link
Contributor

Choose a reason for hiding this comment

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

CTAD does not work here either?

@dalg24
Copy link
Contributor

dalg24 commented Jan 12, 2024

Should probably actually measure performance impact and not just add the GH label

@aprokop
Copy link
Contributor Author

aprokop commented Jan 12, 2024

Although I don't see any issue with that change, you omitted to say that your PR now requires points as input which was not the case before.

It never worked worked correctly with anything but points.

Should probably actually measure performance impact and not just add the GH label

We don't have a neighbor list benchmark.

@aprokop aprokop merged commit 0a62be9 into arborx:master Jan 12, 2024
1 of 2 checks passed
@aprokop aprokop deleted the run_neighborlist_on_bvh_point branch January 12, 2024 15:00
@dalg24
Copy link
Contributor

dalg24 commented Jan 12, 2024

Should probably actually measure performance impact and not just add the GH label

We don't have a neighbor list benchmark.

Crank the size up in the example. 100x100x100 will do on a GPU.

@aprokop
Copy link
Contributor Author

aprokop commented Jan 12, 2024

Crank the size up in the example. 100x100x100 will do on a GPU.

What example? We don't use it in molecular_dynamics example, though I think you had a patch somewhere for it at some point.

@dalg24
Copy link
Contributor

dalg24 commented Jan 12, 2024

Ugggg. I thought we did...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Something is slower than it should be
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants