-
Notifications
You must be signed in to change notification settings - Fork 114
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
LGR: Arithmetic-based localization algorithm #4466
base: master
Are you sure you want to change the base?
Conversation
Jenkins build this please |
1 similar comment
Jenkins build this please |
Please rebase this PR to fix the merge conflict. |
338c4c2
to
1c3ddf6
Compare
jenkins build this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think this will be a nice alternative to the original, and more general, approach.
That said, do please have a look at the definition of floored_div()
inside IJK_location()
. It's not immediately clear to me why we're using the modulus operator here.
ea1df4b
to
f10992e
Compare
…rate_cartesian_product
jenkins build this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks more like I expect it to. A couple of minor issues remain, but I'm getting ready to merge this soon.
std::function<std::size_t(std::size_t, std::size_t)> div = std::divides<std::size_t>{}; | ||
std::function<std::size_t(std::size_t, std::size_t)> sum = std::plus<std::size_t>{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing these in std::function<>
objects introduces a bit of unnecessary overhead. You could just pass
std::divides<>{}
and std::plus<>{}
directly as arguments to vectorScalarOperation()
instead, i.e., as
auto resultI = VectorUtil::vectorScalarOperation(i_list, nx / host_nx, std::divides<std::size_t>{});
resultI = VectorUtil::vectorScalarOperation(resultI, base_host_nx, std::plus<std::size_t>{});
auto [i_list, j_list, k_list] = VectorUtil::generate_cartesian_product(0,nx-1,0, ny-1,0, nz-1); | ||
std::function<std::size_t(std::size_t, std::size_t)> div = std::divides<std::size_t>{}; | ||
std::function<std::size_t(std::size_t, std::size_t)> sum = std::plus<std::size_t>{}; | ||
std::vector<std::size_t> resultI = VectorUtil::vectorScalarOperation(i_list, host_nx, div); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The host_nx
being passed as the scalar
argument here (probably) needs to be nx / host_nx
–i.e., the number of subcells per cell–instead.
std::vector<T> i_list; | ||
std::vector<T> j_list; | ||
std::vector<T> k_list; | ||
std::size_t list_size = (up_nx - low_nx + 1) * (up_ny - low_ny + 1) * (up_nz - low_nz + 1); | ||
i_list.resize(list_size); | ||
j_list.resize(list_size); | ||
k_list.resize(list_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you reorder the lines slightly you can avoid the .resize()
calls here
const auto list_size = ...
auto cart_prod = std::tuple {
std::vector<T>(list_size),
std::vector<T>(list_size),
std::vector<T>(list_size)
};
auto& [i_list, j_list, k_list] = cart_prod;
for (...) {
// ...
}
return cart_prod;
This PR builds upon previous work by introducing a simple arithmetic-based localization algorithm to efficiently determine the host cell for a given LGR cell, improving the speed of the localization process.
Additionally, this PR updates VectorUtil library with a new method for vector-scalar operations, along with other minor utility functions and code refactoring.