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

Fix Multigrid Solver in Forest #1035

Merged
merged 57 commits into from
Apr 17, 2024
Merged

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Mar 26, 2024

PR Summary

This PR updates the geometric multigrid infrastructure to work with the new forest of trees based mesh. Generally, I think this has resulted in a much cleaner implementation.

The forest neighbor finding algorithm is extended to find neighbors in different types of meshes (i.e. the standard leaf mesh and two-level composite meshes at different levels). Additionally, gids are assigned to internal nodes in the forest when Forest::GetMeshBlocksAndResolveGids is called. The way the level of gmg grids is defined is also simplified so that it just corresponds to the logical level of finer blocks that are members of the grid (which means we store the gmg_block_lists in a map instead of a vector since the index can be negative). All of these changes should be internal and not be visible to downstream codes. Because we no longer have to worry about periodic root grids (with periodicity being taken care of by the forest connectivity) and because we use the forest information explicitly for gmg neighbor finding, a lot of the complex neighbor finding code in LogicalLocation is no longer necessary. This PR also removes a lot of other code that died in switching to a forest.

PR Checklist

  • Code passes cpplint
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files
  • [Trivial] Reorganize LogicalLocation code #1028 is merged into develop

@lroberts36 lroberts36 changed the title [WIP] FIx Multigrid solver in Forest [WIP] Fix Multigrid solver in Forest Mar 27, 2024
@lroberts36
Copy link
Collaborator Author

lroberts36 commented Mar 28, 2024

hm, I managed to break something in the last couple commits. Will get back to it on Monday. Likely an issue related to LogicalLocations with negative levels.

@lroberts36
Copy link
Collaborator Author

lroberts36 commented Apr 1, 2024

hm, I managed to break something in the last couple commits. Will get back to it on Monday. Likely an issue related to LogicalLocations with negative levels.

now fixed, was due to a bug introduced by a merge.

@pgrete
Copy link
Collaborator

pgrete commented Apr 11, 2024

Is it safe to change the target branch to develop now given that #1028 has been merged?

@lroberts36 lroberts36 changed the base branch from lroberts36/reorganize to develop April 11, 2024 15:39
@lroberts36 lroberts36 changed the base branch from develop to lroberts36/reorganize April 11, 2024 15:41
@lroberts36 lroberts36 changed the base branch from lroberts36/reorganize to develop April 11, 2024 17:49
@lroberts36
Copy link
Collaborator Author

@pgrete: The target branch is now develop.

Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

I looked at the code and followed as much as I could (which, admittedly is not too much given that I haven't really internalized all the logic and associated magic).
Anyway, negative lines are always welcome and I just stumbled across two numbers that I weren't able to immediate connect.

// lx3() >> nlevel); for most cases
const int norig = 1LL << std::max(level(), 0);
const int nparent = 1LL << std::max(level() - nlevel, 0);
constexpr int nmax_tree_offset = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a magic number/should be be obvious why it's 5?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it could be anything that doesn't overflow when multiplied by norig.

}
}
}
}
}

// Build in negative levels
for (int l = -20; l < 0; ++l) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is -20 just a large enough number that we should never reach it?

Copy link
Collaborator Author

@lroberts36 lroberts36 Apr 17, 2024

Choose a reason for hiding this comment

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

yeah, for that to not be enough would imply we had a meshblock size of (2^20)^D.

block_bcs[GetOuterBoundaryFace(dir)] = mesh_bcs[GetOuterBoundaryFace(dir)];
}
}
block_size = forest.GetBlockDomain(loc);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unused var?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, block_size is passed by reference. The input to this routine is loc and the output is block_size and block_bcs. I agree that this is a weird design, but it is what was originally in the code.

@lroberts36 lroberts36 merged commit c8845f0 into develop Apr 17, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants