-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fix refinement next to refined blocks #928
Changes from 3 commits
1e77d2f
d9820e8
45b64d6
4b5a6df
0291289
5530898
6a472d8
a4e0883
476f0f9
5e649b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -251,15 +251,25 @@ struct block_ownership_t { | |
inline block_ownership_t | ||
DetermineOwnership(const LogicalLocation &main_block, | ||
const std::set<LogicalLocation> &allowed_neighbors, | ||
const RootGridInfo &rg_info = RootGridInfo()) { | ||
const RootGridInfo &rg_info = RootGridInfo(), | ||
const std::set<LogicalLocation> &newly_refined = {}) { | ||
block_ownership_t main_owns; | ||
|
||
auto ownership_less_than = [](const LogicalLocation &a, const LogicalLocation &b) { | ||
auto ownership_level = [&](const LogicalLocation &a) { | ||
// Newly-refined blocks are treated as higher-level than blocks at their | ||
// parent level, but lower-level than previously-refined blocks at their | ||
// current level. | ||
if (newly_refined.count(a)) return 2 * a.level() - 1; | ||
return 2 * a.level(); | ||
}; | ||
|
||
auto ownership_less_than = [ownership_level](const LogicalLocation &a, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. still cheeky IMO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As in, I should collapse it? This seemed pretty easy to understand, if a little over-engineered There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no no---it's fine. Cheeky is not a bad thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no no---it's fine. Cheeky is not a bad thing. |
||
const LogicalLocation &b) { | ||
// Ownership is first determined by block with the highest level, then by maximum | ||
// Morton number this is reversed in precedence from the normal comparators where | ||
// Morton number takes precedence | ||
if (a.level() == b.level()) return a.morton() < b.morton(); | ||
return a.level() < b.level(); | ||
if (ownership_level(a) == ownership_level(b)) return a.morton() < b.morton(); | ||
return ownership_level(a) < ownership_level(b); | ||
}; | ||
|
||
for (int ox1 : {-1, 0, 1}) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -475,11 +475,6 @@ Mesh::Mesh(ParameterInput *pin, ApplicationInput *app_in, Packages_t &packages, | |
} | ||
|
||
ResetLoadBalanceVariables(); | ||
|
||
// Output variables in use in this run | ||
if (Globals::my_rank == 0) { | ||
std::cout << "#Variables in use:\n" << *(resolved_packages) << std::endl; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this included in another PR? Did that not get merged? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, that one's still in queue. I pinged some people, probably it will still go in before this one and I can avoid having to correct my mistake here. |
||
} | ||
|
||
//---------------------------------------------------------------------------------------- | ||
|
@@ -738,11 +733,6 @@ Mesh::Mesh(ParameterInput *pin, ApplicationInput *app_in, RestartReader &rr, | |
} | ||
|
||
ResetLoadBalanceVariables(); | ||
|
||
// Output variables in use in this run | ||
if (Globals::my_rank == 0) { | ||
std::cout << "#Variables in use:\n" << *(resolved_packages) << std::endl; | ||
} | ||
} | ||
|
||
//---------------------------------------------------------------------------------------- | ||
|
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.
I think we may want this to be a
std::unordered_set
for performance reasons.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.
Agreed, I think, the list could potentially be long and we're checking membership.
What's a good hash function for the unordered_set? Just morton number?
Speaking of that, I think I populate this list incorrectly, since I only pull from
nbs
tonbe
of the global block list in populating newly_refined, meaning I need to collate the lists from all ranks. So, also going to fix that.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.
A hash for
LogicalLocation
is injected into thestd::
namespace, sostd::unordered_map<LogicalLocation>
should just work.Good catch. Yeah, I think you need the locations on all ranks.