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

Forest Of Octrees #1009

Merged
merged 119 commits into from
Apr 2, 2024
Merged

Forest Of Octrees #1009

merged 119 commits into from
Apr 2, 2024

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Feb 22, 2024

PR Summary

This PR begins transitioning Parthenon domain from being represented by a single octree to a forest of octrees that cover the mesh domain. This PR switches to a new forest based mesh structure under the hood, but does not provide any user facing mechanism for building generic forests. Rather, it just builds a forest equivalent to the old root grid. Previously, if the base grid wasn't logically cubic parts of the octree domain were masked out. Now, multiple trees will cover the domain without masking. These changes should be transparent to downstream codes. For this limited set of forests, there is a clear way to map between forest logical locations and logical locations in the old Athena++ tree which allows writing the Athena++ locations out to file, building grids based on Athena++ style input, etc. As a result, the behavior of the code and output should be completely unchanged and the changes made in this PR should have no impact on down stream codes (fingers crossed). Obviously, as-is these changes have limited utility but future PRs built on them should allow for more complex grids to be represented.

This PR also removes a couple of big things from the code:

  • MeshblockTree is replaced by Forest and Tree (the latter of which is based on a hash map of logical locations rather than the pointer tree used by MeshblockTree)
  • Most of the old boundary communication machinery (BoundaryBase, BoundaryValue, BoundaryBuffer and BoundaryVariable) and pbval from MeshBlock. There was already a lot of dead code in those classes and this PR required removing the old neighbor finding routines (since they relied on Mesh::tree). To make removing this stuff possible, some reasonably small changes to the swarms related communication was required. The changes and reorganization for this make up a lot of the lines of the PR.

This PR breaks multigrid, but fixing that would make the PR even longer (and no one is using it except me I think).

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

@pgrete
Copy link
Collaborator

pgrete commented Mar 25, 2024

I'll get to review a bit later this week (as I also want to run some [performance] tests downstream)

@lroberts36
Copy link
Collaborator Author

This PR passes all Riot tests.

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.

LGTM -- and also nice cleanup of the code and thanks for the unit tests (which are way more readable than the integrated version of construction in the Mesh class.

I also ran some downstream tests and didn't notice anything odd.
In fact, the AMR performance increased by up to 30%-50% (based on the wsec_AMR output and for a test with several hundreds of blocks per device).

src/bvals/comms/bnd_info.cpp Show resolved Hide resolved
Comment on lines +45 to +81
// TODO(felker): nest these enum definitions inside bvals/ classes, when possible.

// DEPRECATED(felker): maintain old-style (ALL_CAPS) enumerators as unscoped,unnamed types
// Keep for compatibility with user-provided pgen/ files. Use only new types internally.

// GCC 6 added Enumerator Attr (v6.1 released on 2016-04-27)
// TODO(felker): replace with C++14 [[deprecated]] attributes if we ever bump --std=c++14
#if (defined(__GNUC__) && __GNUC__ >= 6) || (defined(__clang__) && __clang_major__ >= 3)
enum {
FACE_UNDEF __attribute__((deprecated)) = -1,
INNER_X1 __attribute__((deprecated)),
OUTER_X1 __attribute__((deprecated)),
INNER_X2 __attribute__((deprecated)),
OUTER_X2 __attribute__((deprecated)),
INNER_X3 __attribute__((deprecated)),
OUTER_X3 __attribute__((deprecated))
};
enum {
BLOCK_BNDRY __attribute__((deprecated)) = -1,
BNDRY_UNDEF __attribute__((deprecated)),
REFLECTING_BNDRY __attribute__((deprecated)),
OUTFLOW_BNDRY __attribute__((deprecated)),
PERIODIC_BNDRY __attribute__((deprecated))
};
#else
enum { FACE_UNDEF = -1, INNER_X1, OUTER_X1, INNER_X2, OUTER_X2, INNER_X3, OUTER_X3 };
enum {
BLOCK_BNDRY = -1,
BNDRY_UNDEF,
REFLECTING_BNDRY,
OUTFLOW_BNDRY,
USER_BNDRY,
PERIODIC_BNDRY,
POLAR_BNDRY,
POLAR_BNDRY_WEDGE
};
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just checked and we don't use it downstream, so this can go.

src/defs.hpp Show resolved Hide resolved
src/mesh/forest/cell_center_offsets.hpp Outdated Show resolved Hide resolved

// TODO(LFR): Consider switching this to C-style enum within a namespace to avoid
// static_cast
enum class Offset : int { Low = -1, Middle = 0, Up = 1 };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Further up in the bvals, the ordering of offset was 0, -1, 1 IIRC.
I realize these are enums here, so it shouldn't matter, but I'm wondering if there any danger for confusion mid/long term wrt to ordering convention.

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 am not sure I see where the ordering is different?

src/mesh/forest/tree.cpp Show resolved Hide resolved
src/mesh/forest/tree.cpp Outdated Show resolved Hide resolved
src/mesh/forest/tree.hpp Show resolved Hide resolved
src/mesh/logical_location.hpp Show resolved Hide resolved
tst/unit/test_forest.cpp Outdated Show resolved Hide resolved
@brryan
Copy link
Collaborator

brryan commented Apr 1, 2024

I don't have any objections to the swarm boundary comms refactor (thank you for working this out @lroberts36!) -- the only thing I would like to do is run the parthenon particle_leapfrog example for a large number of meshblocks and see that it still works. Right now this branch isn't building for me on volta-x86:

"/home/brryan/github/parthenon/example/sparse_advection/sparse_advection_driver.cpp", line 125: error: expected an identifier
      auto restrict =
                    ^

"/home/brryan/github/parthenon/example/sparse_advection/sparse_advection_driver.cpp", line 130: error: expected an expression
        tl.AddTask(restrict, SparseDealloc, mc1.get());

but once it is working again I'm happy to check that example.

Just tested on particle_leapfrog on 32 nodes and it runs and gives the right particle positions at final time, so everything looks good to me.

Copy link
Collaborator

@brryan brryan left a comment

Choose a reason for hiding this comment

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

This approval is mainly for the modifications to particles. Thanks for dealing with that @lroberts36

@lroberts36 lroberts36 disabled auto-merge April 2, 2024 18:27
@lroberts36 lroberts36 merged commit 69a6599 into develop Apr 2, 2024
49 checks passed
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.

Possibly remove MeshBlockTree Refactor bvals code
4 participants