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

Add range minimum query kernels #1764

Merged
merged 44 commits into from
Jan 20, 2025
Merged

Add range minimum query kernels #1764

merged 44 commits into from
Jan 20, 2025

Conversation

upsj
Copy link
Member

@upsj upsj commented Jan 11, 2025

This adds a data structure to answer queries of the kind $\text{argmin}_{i \in [a,b]} \text{array}[i]$ in constant time. This will be used to speed up the computation of LCAs in the symbolic Cholesky algorithm, turning the counting step from $O(nnz(L))$ algorithm to a $O(nnz(A))$ algorithm.

The corresponding Wikipedia articles describe the algorithms pretty well https://en.wikipedia.org/wiki/Range_minimum_query, the constant time query is described in more detail in J. Fischer and V. Heun, “Space-Efficient Preprocessing Schemes for Range Minimum Queries on Static Arrays,” SIAM J. Comput., vol. 40, no. 2, pp. 465–492, Jan. 2011, doi: 10.1137/090779759.

TODO:

  • add test for device queries

@upsj upsj added the 1:ST:ready-for-review This PR is ready for review label Jan 11, 2025
@upsj upsj requested a review from a team January 11, 2025 13:33
@upsj upsj self-assigned this Jan 11, 2025
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:reference This is related to the reference module. mod:hip This is related to the HIP module. labels Jan 11, 2025
@yhmtsai
Copy link
Member

yhmtsai commented Jan 13, 2025

my first impression on turning $O(nnz(L))$ to $O(nnz(A))$ is more operation.
do I miss something?

@upsj
Copy link
Member Author

upsj commented Jan 13, 2025

L includes fill-in, A does not

@MarcelKoch MarcelKoch self-requested a review January 14, 2025 08:32
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

Overall, a lot more documentation is necessary. Especially what the new classes in bit_packed_storage and range_minumum_querry are there for, and how they work together.
At least I can't properly review it right now, since there is too little information on what functions/classes are supposed to do.

include/ginkgo/core/base/intrinsics.hpp Outdated Show resolved Hide resolved
core/components/bit_packed_storage.hpp Outdated Show resolved Hide resolved
core/components/bit_packed_storage.hpp Outdated Show resolved Hide resolved
core/components/bit_packed_storage.hpp Outdated Show resolved Hide resolved
core/components/bit_packed_storage.hpp Outdated Show resolved Hide resolved
common/unified/components/range_minimum_query_kernels.cpp Outdated Show resolved Hide resolved
reference/test/components/range_minimum_query_kernels.cpp Outdated Show resolved Hide resolved
@upsj upsj requested review from MarcelKoch and a team January 19, 2025 21:44
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

I would like to have the discussion on the description of lookup kernels resolved. The rest are smaller nits.

core/components/bit_packed_storage.hpp Outdated Show resolved Hide resolved
core/components/bit_packed_storage.hpp Outdated Show resolved Hide resolved
core/components/bit_packed_storage.hpp Outdated Show resolved Hide resolved
}
}

/** Returns the ballot number C(p, q). */
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to describe in a few words the meaning of C(p, q) in this context?

Copy link
Member Author

@upsj upsj Jan 20, 2025

Choose a reason for hiding this comment

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

I don't think there is any particularly useful description I could add here, beyond the definition above.

core/components/range_minimum_query.hpp Outdated Show resolved Hide resolved
core/components/range_minimum_query_kernels.hpp Outdated Show resolved Hide resolved
@upsj upsj added 1:ST:ready-to-merge This PR is ready to merge. 1:ST:no-changelog-entry Skip the wiki check for changelog update and removed 1:ST:ready-for-review This PR is ready for review 1:ST:run-full-test labels Jan 20, 2025
@upsj upsj merged commit efbf245 into develop Jan 20, 2025
10 of 11 checks passed
@upsj upsj deleted the rmq branch January 20, 2025 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:no-changelog-entry Skip the wiki check for changelog update 1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:hip This is related to the HIP module. mod:reference This is related to the reference module. reg:build This is related to the build system. reg:testing This is related to testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants