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

Switch the signature for the pure callback #900

Merged
merged 3 commits into from
Jul 21, 2023
Merged

Switch the signature for the pure callback #900

merged 3 commits into from
Jul 21, 2023

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Jun 19, 2023 •
edited

This PR does not touch the callbacks related to the CrsGraphWrapper.

@aprokop aprokop added the API User visible interface modifications label Jun 19, 2023
@aprokop aprokop mentioned this pull request Jun 19, 2023
Open
dalg24
dalg24 reviewed Jul 17, 2023
View reviewed changes
test/tstCompileOnlyTypeRequirements.cpp Outdated Show resolved Hide resolved
src/details/ArborX_Callbacks.hpp Show resolved Hide resolved
test/tstDetailsMutualReachabilityDistance.cpp Outdated Show resolved Hide resolved
src/details/ArborX_Callbacks.hpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsFDBSCANDenseBox.hpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsFDBSCANDenseBox.hpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsFDBSCANDenseBox.hpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsFDBSCANDenseBox.hpp Outdated Show resolved Hide resolved
@aprokop
Copy link
Contributor Author

aprokop commented Jul 17, 2023

Rebased on master so that we don't constantly trigger container rebuild. First three commits are the same as prior to the review.

dalg24
dalg24 reviewed Jul 17, 2023
View reviewed changes
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.

What about check_valid_callback_if_first_argument_is_not_a_view?

src/ArborX_BruteForce.hpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsFDBSCANDenseBox.hpp Outdated Show resolved Hide resolved
src/details/ArborX_Callbacks.hpp Show resolved Hide resolved
masterleinad
masterleinad reviewed Jul 17, 2023
View reviewed changes
Copy link
Collaborator

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Any chance to get some more tests with a distinct Value type?

@aprokop
Copy link
Contributor Author

aprokop commented Jul 17, 2023 •
edited

Any chance to get some more tests with a distinct Value type?

Should do that. But maybe as a separate PR? And what exactly do you mean by a distinct Value type?
Currently, I have a follow up patch that uses Value = PairIndexVolume<Point> and BoundingVolume = Box for DBSCAN.

@aprokop
Copy link
Contributor Author

aprokop commented Jul 18, 2023

What about check_valid_callback_if_first_argument_is_not_a_view?

It only happens in CrsGraphWrapper. As I didn't modify the 3-arg callback version, I did not need to change the check.

@aprokop aprokop mentioned this pull request Jul 18, 2023
Closed
@aprokop
Copy link
Contributor Author

aprokop commented Jul 19, 2023

No difference in performance benchmark (here).

aprokop
aprokop commented Jul 19, 2023
View reviewed changes
src/details/ArborX_DetailsCrsGraphWrapperImpl.hpp
Comment on lines +67 to 78
// Legacy callback wrapper
template <typename Value, bool B = Legacy,
typename Enable = std::enable_if_t<!B>>
KOKKOS_FUNCTION auto operator()(PredicateType const &predicate,
Value const &value) const
{
return (*this)(predicate, (int)value.index);
}

KOKKOS_FUNCTION auto operator()(PredicateType const &predicate,
int primitive_index) const
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still iffy. I think this will only work with BasicBoundingVolumeHierarchy that has Value hardcoded to PairIndexVolume. I hope to be able to switch the signature of the crs graph in the future, this should be the last hard task.

dalg24
dalg24 reviewed Jul 19, 2023
View reviewed changes
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.

I am ok with PR pending Daniel's approval (hence comment instead of formal approve)
Please squash merge

@masterleinad
Copy link
Collaborator

I am ok with PR pending Daniel's approval (hence comment instead of formal approve)

I would prefer if we could document some more what these changes actually mean in practice either with an example or a pull request to https://github.com/arborx/Wiki but I didn't mind if that's done in a follow-up.

@aprokop
Copy link
Contributor Author

aprokop commented Jul 20, 2023

Please squash merge

I would prefer to have at least 3 commits here (like the first 3). I can rebase to integrate the reviews (last 2 commits) into them if you prefer that.

@aprokop
Copy link
Contributor Author

aprokop commented Jul 20, 2023

I would prefer if we could document some more what these changes actually mean in practice either with an example or a pull request to arborx/Wiki but I didn't mind if that's done in a follow-up.

We'll do that. My first priority is getting everything in. I think I've scoped everything and know the path, but there may be some corner cases I still haven't considered. So I'm hesitant to put work into documentation right now until we get everything in, are happy with the final result, and made sure all our use cases work fine with it.

@dalg24
Copy link
Contributor

dalg24 commented Jul 20, 2023

Please squash merge

I would prefer to have at least 3 commits here (like the first 3). I can rebase to integrate the reviews (last 2 commits) into them if you prefer that.

ok with cleanup

@aprokop
Copy link
Contributor Author

aprokop commented Jul 21, 2023

Everything passes except HIP which did not run.

@aprokop aprokop merged commit 95bd0c0 into arborx:master Jul 21, 2023
1 check failed
@aprokop aprokop deleted the 8-update_callback branch July 21, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 left review comments

@masterleinad masterleinad masterleinad approved these changes

Assignees
No one assigned
Labels
API User visible interface modifications
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@aprokop @masterleinad @dalg24