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

Remove multiplet number estimator and use the counter result to resize the multiplet container #110

Merged
merged 10 commits into from
Jan 21, 2022

Conversation

beomki-yeo
Copy link
Contributor

This PR addresses #93.
The size of multiplet (doublet, triplet, and seed) container used to be resized based on the empirical values obtained by cpu seed finding. The changes in this PR makes seed finding no longer rely on the empirical values but use the counting result to resize the container.

To make it possible, the counter objects had to be extended to include the more information on the number of multiplets.

Good thing is that the overall speedup gets improved because the size of doublet and triplet container is not over-estimated anymore:

Cuda seeding Elapsed time for 20 events
Before (1.4 sec) -> After (1.0 sec)

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Formally I think this looks fine.

I was very confused by the doublet_per_bin and triplet_per_bin structs at first. Since those are really just an unnecessary complication over simply using an unsigned int by itself, like the code has done before.

I see that the interface must've been developed for the dublet_counter_per_bin struct at first, where a struct is actually meaningful. But... Is this uniform interface for these 3 structs actually important at any point? Did you write some generic code somewhere that would use these 3 types in a uniform way? I can't see that in the code... (Though I might just be missing it.)

As always, C++20 concepts would fit this type of code beautifully. Hopefully we'll get those in not too long...

core/include/seeding/detail/doublet.hpp Outdated Show resolved Hide resolved
@beomki-yeo
Copy link
Contributor Author

beomki-yeo commented Jan 19, 2022

Did you write some generic code somewhere that would use these 3 types in a uniform way?

Yeah. They have uniform structure to use get_ref_num() in cuda/utils/cuda_helper.cuh :p

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Once #108 is merged in, this should still be updated to include those changes first. That should also give the CI a chance to fix the (hopefully temporary) hiccup that occurred in the last run.

But apart from these technicalities, I'm on board with this PR as well.

@beomki-yeo
Copy link
Contributor Author

I want to merge this - but why review-required: main is not satisfied although it was already approved? @paulgessinger

@paulgessinger
Copy link
Member

The review through the bot gets invalidated when you add new commits to the branch that are not just update commits.

@paulgessinger
Copy link
Member

Should be green now

@beomki-yeo
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants