Add reduction action for AMR diagnostics #4759
Conversation
| using return_tags = | ||
| tmpl::list<amr::Tags::Flags<Dim>, amr::Tags::NeighborFlags<Dim>>; | ||
| /// Tags for constant items added to the GlobalCache. These items are | ||
| /// initialized from input file options. |
There was a problem hiding this comment.
We shouldn't (have to) add these explanations to every initialization mutator. How about adding a protocol that defines these aliases?
There was a problem hiding this comment.
I have no objection to someone else doing that...
There was a problem hiding this comment.
In this PR it would be enough to (1) delete these comments that we shouldn't (have to) duplicate in every initializer, and (2) link to InitializeItems in the docs (that's super important, else it's unclear what all these aliases even mean and why they're there). I opened #4761 for the rest.
Not sure what you mean with "no objections to someone else doing that". In my opinion we can't expect others to resolve issues like this. Happy to discuss in a call if you like.
| template <size_t Dim> | ||
| struct Domain { | ||
| /// Tags for constant items added to the GlobalCache. These items are | ||
| /// initialized from input file options. |
There was a problem hiding this comment.
Same here. This verbosity / boilerplate code seems excessive. We can't document what each of these aliases mean in every initializer. It also distracts from the actual implementation in this file. A protocol would solve this I think.
| std::array{Parallel::Phase::Initialization, Parallel::Phase::CheckDomain, | ||
| Parallel::Phase::Exit}; | ||
|
|
||
| /// The parallel components used in the executable |
| /// - The average refinement level by logical dimension (i.e. not by the | ||
| /// physical dimensions) | ||
| /// - The average number of grid points by logical dimension | ||
| struct UseAmrDiagnostics { |
There was a problem hiding this comment.
CheckAmrDiagnostics? Not sure what "use" means.
There was a problem hiding this comment.
That's what I had named it when it just checked the Block volume. But then I didn't think that was best after I started doing additional things. And I can easily imagine more things being done as we start to use AMR. @nilsdeppe @wthrowe thoughts on the name?
There was a problem hiding this comment.
What about RunAmrDiagnostics?
| @@ -0,0 +1,93 @@ | |||
| // Distributed under the MIT License. | |||
There was a problem hiding this comment.
Should this executable go in tests/?
There was a problem hiding this comment.
It could, but I thought it could be a good example of a parallel executable that can easily be run on multiple nodes and cores, and used in a tutorial. This is not the final state of the executable, it will do full h-refinement. I just added it now to test the reduction action.
There was a problem hiding this comment.
ok, I'm not sure if Examples/ or tests/ is the right place. Either seems fine I think. @wthrowe @nilsdeppe any opinions?
| /// | ||
| /// \note This only initializes the Element and the Mesh on each element of | ||
| /// an array component. This is all that is needed about the Domain in order to | ||
| /// test the mechanics of of adaptive mesh refinement. |
| ::domain::Tags::InitialRefinementLevels<Dim>, | ||
| evolution::dg::Tags::Quadrature>; | ||
|
|
||
| using default_initialized_simple_tags = tmpl::list<>; |
There was a problem hiding this comment.
I don't see this being used anywhere, and I don't think it is part of some generic interface (or is it?). This confusion is what I think we must fix (#4761). For the purpose of this PR just delete this line?
| @@ -0,0 +1,93 @@ | |||
| // Distributed under the MIT License. | |||
There was a problem hiding this comment.
ok, I'm not sure if Examples/ or tests/ is the right place. Either seems fine I think. @wthrowe @nilsdeppe any opinions?
| using mutable_global_cache_tags = tmpl::list<>; | ||
| using simple_tags_from_options = tmpl::list<>; | ||
| using default_initialized_simple_tags = | ||
| tmpl::list<amr::Tags::NeighborFlags<Dim>>; |
There was a problem hiding this comment.
just inline this in simple_tags below? As noted above, I think this isn't part of a generic interface.
There was a problem hiding this comment.
where do you want it documented that this adds a default initialized tag into the DataBox?
There was a problem hiding this comment.
that's part of the simple_tags interface, isn't it? That should be documented somewhere. I find the overlap between the InitializeItems interface and the simple_tags / compute_tags interface a bit confusing here, but nothing to fix in this PR.
Change from the type aliases expected by Initialization::Actions::AddSimpleTags to those expected by Initialization::Actions::InitializeItems.
As this action is a reduction action, also add the executable RandomAmr that uses it. After more AMR actions are added RandomAmr can be used as a integration test of AMR actions and components.
kidder
force-pushed
the
amr_diagnostics
branch
from
ffa0273
to
44c6389
Compare
Proposed changes
This allows (among other things):
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments