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

Add a MeshData variant for refinement tagging #1182

Open
wants to merge 54 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
6814173
meshdata version of refinement tagging
acreyes Sep 26, 2024
6df75e7
got initialization and vector indices working
acreyes Sep 26, 2024
2fdc2ed
fix tensor indices
acreyes Sep 27, 2024
5cb25b4
added CheckRefinementMesh to fine-advection example
acreyes Sep 27, 2024
3ad74e1
cleaning up var names
acreyes Sep 28, 2024
16a1c58
cleanup
acreyes Sep 28, 2024
6e1b905
adding scatter view utilities
acreyes Sep 28, 2024
2b6545c
scatterview version of refinement
acreyes Sep 28, 2024
ccf72b0
burgers-benchmark uses Tag<MeshData>
acreyes Sep 29, 2024
d7d536f
add hierarchial par
acreyes Sep 29, 2024
fa37fb9
cleanup includes
acreyes Sep 29, 2024
401f412
missed one
acreyes Sep 29, 2024
638e077
Update CHANGELOG.md
acreyes Sep 29, 2024
b6e0e6b
remove default level tag
acreyes Sep 29, 2024
2b3f32a
default CheckRefineMesh to true
acreyes Sep 29, 2024
622882d
respect amr_criteria max_level
acreyes Sep 29, 2024
5b7863b
renaming delta_levels->amr_tags, mc->md
acreyes Sep 30, 2024
06e0ade
fix refinement/bc order
acreyes Sep 30, 2024
3cf589d
docs for CheckRefinementMesh
acreyes Sep 30, 2024
7ea604c
move amr_tags array to mesh
acreyes Oct 1, 2024
d2a92b0
Merge branch 'develop' into acreyes/meshdata-refinement
Yurlungur Oct 14, 2024
dd4a652
adding comments for ScatterMax view
acreyes Oct 2, 2024
fe95b47
it compiles at least
jonahm-LANL Sep 13, 2024
554a1d4
add easy machinery to register reflecting BCs
jonahm-LANL Sep 13, 2024
781d032
changelog
acreyes Nov 8, 2024
c1beb9e
swarm bcs differetn from mesh bcs
jonahm-LANL Sep 13, 2024
9e8b17f
use new input block
jonahm-LANL Sep 13, 2024
bcbc1c7
typo
jonahm-LANL Sep 27, 2024
b2a5e52
add error checking for swarm/mesh BC consistency
jonahm-LANL Sep 27, 2024
46517ab
typo
jonahm-LANL Sep 27, 2024
f9313d3
phdf diff
jonahm-LANL Sep 28, 2024
20abf5a
Register reflecting BCs for advection examples
jonahm-LANL Sep 28, 2024
1408bf0
typo
jonahm-LANL Sep 28, 2024
5b91a7a
silly backwards compatibility thing to make it so you don't have to s…
jonahm-LANL Sep 28, 2024
382a6a7
working
lroberts36 Oct 10, 2024
4ee02c5
changelog
lroberts36 Oct 10, 2024
862c2a9
Make everything work
lroberts36 Oct 10, 2024
bd50d24
format
lroberts36 Oct 10, 2024
09464c2
maybe fix doc issue?
lroberts36 Oct 10, 2024
df06a50
Address CUDA MPI/ICP issue with Kokkos <=4.4.1 (#1189)
pgrete Oct 15, 2024
511c77c
Jonah's fix for this CI issue
brryan Oct 31, 2024
328dc33
CHANGELOG
brryan Oct 31, 2024
5e657c0
Remove else from if constexpr when there are returns
adamdempsey90 Oct 31, 2024
dd928ab
Consolidate buffer packing functions with less atomics (#1199)
alexrlongne Nov 1, 2024
0cbbdac
[Trivial] Fix type used for array init (#1170)
pgrete Nov 4, 2024
01517f7
Leapfrog fix (#1206)
brryan Nov 8, 2024
3ddbb9f
removing meshblock amr_criteria
acreyes Nov 8, 2024
454324f
Merge remote-tracking branch 'upstream/develop' into acreyes/meshdata…
acreyes Nov 14, 2024
9cdf3aa
Merge remote-tracking branch 'upstream/develop' into acreyes/meshdata…
acreyes Nov 15, 2024
ed3e16f
use pack.UpperBound(b) to check allocation
acreyes Nov 15, 2024
cc01266
Merge branch 'develop' into acreyes/meshdata-refinement
pgrete Nov 25, 2024
5a2470b
Merge branch 'develop' into acreyes/meshdata-refinement
Yurlungur Nov 25, 2024
cc97120
remove meshblock first/second derivative from cpp
acreyes Nov 25, 2024
9338c54
linting
acreyes Nov 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Current develop

### Added (new features/APIs/variables/...)
- [[PR 1182]](https://github.com/parthenon-hpc-lab/parthenon/pull/1182) Add a MeshData variant for refinement tagging
- [[PR 1179]](https://github.com/parthenon-hpc-lab/parthenon/pull/1179) Make a global variable for whether simulation is a restart
- [[PR 1171]](https://github.com/parthenon-hpc-lab/parthenon/pull/1171) Add PARTHENON_USE_SYSTEM_PACKAGES build option
- [[PR 1161]](https://github.com/parthenon-hpc-lab/parthenon/pull/1161) Make flux field Metadata accessible, add Metadata::CellMemAligned flag, small perfomance upgrades
Expand Down
10 changes: 2 additions & 8 deletions benchmarks/burgers/burgers_driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ TaskCollection BurgersDriver::MakeTaskCollection(BlockList_t &blocks, const int
// estimate next time step
if (stage == integrator->nstages) {
auto new_dt = tl.AddTask(update, EstimateTimestep<MeshData<Real>>, mc1.get());
auto tag_refine =
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might need a if (pmesh->adaptive) { here, don't we?

Also, conceptually, we previously checked for refinement after the (physical) boundary conditions were set, which I think is more appropriate.

Independent of this PR , it just occurred to me that we're setting the timestep inside the Step() of the driver and only outside do the actual refinement.
Doesn't this potentially cause issues with violating the cfl on fine blocks after a block was refined?
Also pinging @jdolence @lroberts36 @Yurlungur @bprather here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we do need pmesh->adaptive.

Regarding the time step control... I agree in principle that could be a problem but we never encountered any issues, so I wonder if there's something I'm missing there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we do need pmesh->adaptive.

Agreed. Its fixed now and I also moved to the ApplyBoundaryConditionsMD to keep everything all in the same task region to tag after applying the BC.

Regarding the time step control... I agree in principle that could be a problem but we never encountered any issues, so I wonder if there's something I'm missing there.

I think because the refined variable is 10x all the other components it dominates the CFL condition. As a little experiment I offset the spatial profile of the non-refined components and gave one of them a 50x multiplier to try and force where the timestep is overestimated before a refinement. There is definitely some non-monotonicity compared to uniformly refined case.
image
image

tl.AddTask(update, parthenon::Refinement::Tag<MeshData<Real>>, mc1.get());
}
}

Expand All @@ -135,14 +137,6 @@ TaskCollection BurgersDriver::MakeTaskCollection(BlockList_t &blocks, const int

// set physical boundaries
auto set_bc = tl.AddTask(none, parthenon::ApplyBoundaryConditions, sc1);

if (stage == integrator->nstages) {
// Update refinement
if (pmesh->adaptive) {
auto tag_refine = tl.AddTask(
set_bc, parthenon::Refinement::Tag<MeshBlockData<Real>>, sc1.get());
}
}
}
return tc;
}
Expand Down
49 changes: 48 additions & 1 deletion example/fine_advection/advection_package.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,59 @@ std::shared_ptr<StateDescriptor> Initialize(ParameterInput *pin) {
pkg->AddField<Conserved::divD>(
Metadata({Metadata::Cell, Metadata::Derived, Metadata::OneCopy}));

pkg->CheckRefinementBlock = CheckRefinement;
bool check_refine_mesh =
pin->GetOrAddBoolean("parthenon/mesh", "CheckRefineMesh", true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should not mix downstream/example parameters with Parthenon intrinsic parameters, i.e., CheckRefineMesh should belong in the <Advection> input block (and maybe also get a short commend, that his is an example usage for academic/test purposes and that downstream codes typically only use either or -- with a strong recommendation for the MeshData variant)

if (check_refine_mesh) {
pkg->CheckRefinementMesh = CheckRefinementMesh;
} else {
pkg->CheckRefinementBlock = CheckRefinement;
}
pkg->EstimateTimestepMesh = EstimateTimestep;
pkg->FillDerivedMesh = FillDerived;
return pkg;
}

void CheckRefinementMesh(MeshData<Real> *md,
parthenon::ParArray1D<AmrTag> &delta_levels) {
// refine on advected, for example. could also be a derived quantity
static auto desc = parthenon::MakePackDescriptor<Conserved::phi>(md);
auto pack = desc.GetPack(md);

auto pkg = md->GetMeshPointer()->packages.Get("advection_package");
const auto &refine_tol = pkg->Param<Real>("refine_tol");
const auto &derefine_tol = pkg->Param<Real>("derefine_tol");

auto ib = md->GetBoundsI(IndexDomain::entire);
auto jb = md->GetBoundsJ(IndexDomain::entire);
auto kb = md->GetBoundsK(IndexDomain::entire);
auto scatter_levels = delta_levels.ToScatterView<Kokkos::Experimental::ScatterMax>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, and for the other calls: We might need a reset() call here, don't we?

Also kokkos/kokkos#6363 is not an issue here because the delta_levels are reset to derefine on any call anyway, aren't they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand what reset() does. Is it for the values in the ScatterView, but doesn't effect the original view?

Also kokkos/kokkos#6363 is not an issue here because the delta_levels are reset to derefine on any call anyway, aren't they?

yes, they are reset at the inital call to CheckAllRefinement and accumulated through all the packages & criteria

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand what reset() does. Is it for the values in the ScatterView, but doesn't effect the original view?

That is a very good question. I just followed the example on p120 of file:///home/pgrete/Downloads/KokkosTutorial_ORNL20.pdf when I implemented the scatterview for the histograms.

I'm going to ask on the Kokkos Slack.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, I got some info:

Stan Moore
  Yesterday at 9:09 PM
Use reset() if you want to zero out all the values in the scatter view, e.g. you used it in one kernel and now want to use it in another. Note that there is also the reset_except() which can preserve the values in the original view, e.g. see https://github.com/lammps/lammps/blob/develop/src/KOKKOS/fix_qeq_reaxff_kokkos.cpp#L875C5-L876C31


Philipp Grete
  Today at 8:42 PM
I see. So just to double check for our use case (which is a view with N elements that by default is set to -1. We need to update that view every cycle, so we currently (re)set/deep_copy -1 to the view at the beginning of each cycle and then use a scatterview to update that view in every cycle. The kernel using the scatterview may be called multiple times, but always for distinct elements of N):
I should call scatterview.reset_except(original_view) so that we keep the -1 and also only call it once after we set the values in the original view (rather than before each kernel launch) so that the data in the scatter view remains consistent across kernel launches. Moreover, we only need to call contribute once after all the kernels ran.
Am I missing sth.?


Stan Moore
  9 minutes ago
> I should call scatterview.reset_except(original_view) so that we keep the -1 and also only call it once after we set the values in the original view (rather than before each kernel launch) so that the data in the scatter view remains consistent across kernel launches.
If the ScatterView persists across cycles, then yes you need to reset it. And yes you need to use reset_except so the -1 values are not overwritten in the original view. If it is reallocated at the beginning of every cycle, the values of the extra copies are already zero-initialized on creation by default, so no reset would be needed.
> Moreover, we only need to call contribute once after all the kernels ran.
Yes the values will persist as long as you don't call reset. (edited) 

So, with the current pattern, it seems that we need a reset_except (and contribute) outside the loop launching the check refinement kernel over MeshData.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome, thanks @pgrete!

So, with the current pattern, it seems that we need a reset_except (and contribute) outside the loop launching the check refinement kernel over MeshData.

As it is now the ScatterView is created before each kernel and contributed after. I don't think the reset is necessary since the ScatterViews don't persist in between the kernel launches.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think so, too.

parthenon::par_for_outer(
PARTHENON_AUTO_LABEL, 0, 0, 0, pack.GetNBlocks() - 1, 0,
pack.GetMaxNumberOfVars() - 1, kb.s, kb.e,
KOKKOS_LAMBDA(parthenon::team_mbr_t team_member, const int b, const int n,
const int k) {
typename Kokkos::MinMax<Real>::value_type minmax;
par_reduce_inner(
parthenon::inner_loop_pattern_ttr_tag, team_member, jb.s, jb.e, ib.s, ib.e,
[&](const int j, const int i,
typename Kokkos::MinMax<Real>::value_type &lminmax) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any specific reason for the b,n,k / j,i split?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

par_for_outer only has overloads up to 3D was the constraint here. #1142 would allow more flexibility.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. That makes sense. In that case, I suggest to open an issue (before merging) this to keep track of updating these functions once #1142 is in.

lminmax.min_val = (pack(n, k, j, i) < lminmax.min_val ? pack(n, k, j, i)
: lminmax.min_val);
lminmax.max_val = (pack(n, k, j, i) > lminmax.max_val ? pack(n, k, j, i)
: lminmax.max_val);
},
Kokkos::MinMax<Real>(minmax));

auto levels_access = scatter_levels.access();
auto flag = AmrTag::same;
if (minmax.max_val > refine_tol && minmax.min_val < derefine_tol)
flag = AmrTag::refine;
if (minmax.max_val < derefine_tol) flag = AmrTag::derefine;
levels_access(b).update(flag);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work as expected (in particular with the split across k, and j/i (similarly below for the derivs)?
If we refine just a single cell in a block, this info should be persistent even if other cells in that block is same or derefine, so currently there could be a clash of info across ks doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so. It should be equivalent to doing

Kokkos::atomic_max(&delta_levels(b), flag);

So the race condition is across ks and the max ensures that refinement always wins

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right, yes. I didn't see see. Might be worth adding a comment in that direction (at least to the common/non-example function in src/amr_criteria/refinement_package.cpp.

});
delta_levels.ContributeScatter(scatter_levels);
}

AmrTag CheckRefinement(MeshBlockData<Real> *rc) {
// refine on advected, for example. could also be a derived quantity
static auto desc = parthenon::MakePackDescriptor<Conserved::phi>(rc);
Expand Down
1 change: 1 addition & 0 deletions example/fine_advection/advection_package.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ VARIABLE(advection, divD);

std::shared_ptr<StateDescriptor> Initialize(ParameterInput *pin);
AmrTag CheckRefinement(MeshBlockData<Real> *rc);
void CheckRefinementMesh(MeshData<Real> *md, parthenon::ParArray1D<AmrTag> &delta_levels);
Real EstimateTimestep(MeshData<Real> *md);
TaskStatus FillDerived(MeshData<Real> *md);

Expand Down
40 changes: 40 additions & 0 deletions src/amr_criteria/amr_criteria.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,26 @@ AmrTag AMRFirstDerivative::operator()(const MeshBlockData<Real> *rc) const {
return Refinement::FirstDerivative(bnds, q, refine_criteria, derefine_criteria);
}

void AMRFirstDerivative::operator()(MeshData<Real> *mc,
ParArray1D<AmrTag> &delta_levels) const {
auto ib = mc->GetBoundsI(IndexDomain::interior);
auto jb = mc->GetBoundsJ(IndexDomain::interior);
auto kb = mc->GetBoundsK(IndexDomain::interior);
auto bnds = AMRBounds(ib, jb, kb);
auto dims = mc->GetMeshPointer()->resolved_packages->FieldMetadata(field).Shape();
int n5(0), n4(0);
if (dims.size() > 2) {
n5 = dims[1];
n4 = dims[2];
} else if (dims.size() > 1) {
n5 = dims[0];
n4 = dims[1];
}
const int idx = comp4 + n4 * (comp5 + n5 * comp6);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we don't have a simpler way to get a flat index (I imagine there must be more places with thus use case but I don't recall offhand).

Refinement::FirstDerivative(bnds, mc, field, idx, delta_levels, refine_criteria,
derefine_criteria, max_level);
}

AmrTag AMRSecondDerivative::operator()(const MeshBlockData<Real> *rc) const {
if (!rc->HasVariable(field) || !rc->IsAllocated(field)) {
return AmrTag::same;
Expand All @@ -105,4 +125,24 @@ AmrTag AMRSecondDerivative::operator()(const MeshBlockData<Real> *rc) const {
return Refinement::SecondDerivative(bnds, q, refine_criteria, derefine_criteria);
}

void AMRSecondDerivative::operator()(MeshData<Real> *mc,
ParArray1D<AmrTag> &delta_levels) const {
auto ib = mc->GetBoundsI(IndexDomain::interior);
auto jb = mc->GetBoundsJ(IndexDomain::interior);
auto kb = mc->GetBoundsK(IndexDomain::interior);
auto bnds = AMRBounds(ib, jb, kb);
auto dims = mc->GetMeshPointer()->resolved_packages->FieldMetadata(field).Shape();
int n5(0), n4(0);
if (dims.size() > 2) {
n5 = dims[1];
n4 = dims[2];
} else if (dims.size() > 1) {
n5 = dims[0];
n4 = dims[1];
}
const int idx = comp4 + n4 * (comp5 + n5 * comp6);
Refinement::SecondDerivative(bnds, mc, field, idx, delta_levels, refine_criteria,
derefine_criteria, max_level);
}

} // namespace parthenon
4 changes: 4 additions & 0 deletions src/amr_criteria/amr_criteria.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include "defs.hpp"
#include "mesh/domain.hpp"
#include "mesh/mesh.hpp"

namespace parthenon {

Expand All @@ -36,6 +37,7 @@ struct AMRCriteria {
AMRCriteria(ParameterInput *pin, std::string &block_name);
virtual ~AMRCriteria() {}
virtual AmrTag operator()(const MeshBlockData<Real> *rc) const = 0;
virtual void operator()(MeshData<Real> *mc, ParArray1D<AmrTag> &delta_level) const = 0;
std::string field;
Real refine_criteria, derefine_criteria;
int max_level;
Expand All @@ -49,12 +51,14 @@ struct AMRFirstDerivative : public AMRCriteria {
AMRFirstDerivative(ParameterInput *pin, std::string &block_name)
: AMRCriteria(pin, block_name) {}
AmrTag operator()(const MeshBlockData<Real> *rc) const override;
void operator()(MeshData<Real> *mc, ParArray1D<AmrTag> &delta_level) const override;
};

struct AMRSecondDerivative : public AMRCriteria {
AMRSecondDerivative(ParameterInput *pin, std::string &block_name)
: AMRCriteria(pin, block_name) {}
AmrTag operator()(const MeshBlockData<Real> *rc) const override;
void operator()(MeshData<Real> *mc, ParArray1D<AmrTag> &delta_level) const override;
};

} // namespace parthenon
Expand Down
152 changes: 146 additions & 6 deletions src/amr_criteria/refinement_package.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,25 @@
#include <utility>

#include "amr_criteria/amr_criteria.hpp"
#include "interface/make_pack_descriptor.hpp"
#include "interface/mesh_data.hpp"
#include "interface/meshblock_data.hpp"
#include "interface/state_descriptor.hpp"
#include "kokkos_abstraction.hpp"
#include "mesh/mesh.hpp"
#include "mesh/mesh_refinement.hpp"
#include "mesh/meshblock.hpp"
#include "parameter_input.hpp"
#include "utils/instrument.hpp"

namespace parthenon {
namespace Refinement {

std::shared_ptr<StateDescriptor> Initialize(ParameterInput *pin) {
auto ref = std::make_shared<StateDescriptor>("Refinement");
bool check_refine_mesh =
pin->GetOrAddBoolean("parthenon/mesh", "CheckRefineMesh", true);
ref->AddParam("check_refine_mesh", check_refine_mesh);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I now see that parthenon/mesh/CheckRefineMesh is also being used outside of the example.
Personally, I'm not a super big fan of these switches for callback functions (for most other callback functions we only allow to enroll the MeshData version or the MeshBlockData and check if both are enrolled [and then throw an error]).
I'd be in favor of removing the input file switch and go with that pattern, but more people should weigh in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main use is for deciding which amr_criteria variant to use, rather than for the callbacks. Maybe there is a better name to avoid the confusion, something like MeshDataCriteria. I was avoiding removing the MeshBlockData derivative criteria, but it shouldn't be breaking.

As it is written nothing checks whether both callbacks are enrolled, but I can add that as you've described. I only used the switch to easily check that it was giving the same refinement patterns.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be nice to get additional input here (@lroberts36 @jdolence @bprather @brryan @Yurlungur ) with regard to whether this a pattern we generally want to introduce to the codebase.
I'm in favor of (potentially) breaking backwards compatibility (though I think it should also work without breaking) over introduce a runtime switch that determines the callback function (at the Parthenon level).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would vote for not introducing a runtime parameter that switches the callback function. I think we should be moving to using MeshData based functions everywhere and encouraging downstream users to do this as well, so I would also support removing the MeshBlockData based criteria as well (unless this ends up putting a significant burden on some downstream codes?).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Entirely removing the MeshBlockData callback would be breaking. In riot at least that would require some effort, though not a lot, to conform. I think that would be better in the long term though and it wouldn't be too big of a burden, assuming MeshData is alwasy faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The runtime switch isn't for the callbacks, but rather for the refinement criteria. The callbacks are treated the same as the EstimateTimeStep callbacks.

If we don't have the runtime switch then I think the MeshBlockData based amr_criteria would need to be removed. Unless there is some other way to keep both but not call both in each cycle.


int numcrit = 0;
while (true) {
Expand All @@ -48,7 +54,32 @@ std::shared_ptr<StateDescriptor> Initialize(ParameterInput *pin) {
return ref;
}

AmrTag CheckAllRefinement(MeshBlockData<Real> *rc) {
ParArray1D<AmrTag> CheckAllRefinement(MeshData<Real> *mc) {
const int nblocks = mc->NumBlocks();
// maybe not great to allocate this all the time
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed. How about making this a private Mesh member?

auto delta_levels = ParArray1D<AmrTag>(Kokkos::View<AmrTag *>(
Kokkos::view_alloc(Kokkos::WithoutInitializing, "delta_levels"), nblocks));
Kokkos::deep_copy(delta_levels.KokkosView(), AmrTag::derefine);

Mesh *pm = mc->GetMeshPointer();
static const bool check_refine_mesh =
pm->packages.Get("Refinement")->Param<bool>("check_refine_mesh");

for (auto &pkg : pm->packages.AllPackages()) {
auto &desc = pkg.second;
desc->CheckRefinement(mc, delta_levels);

if (check_refine_mesh) {
for (auto &amr : desc->amr_criteria) {
(*amr)(mc, delta_levels);
}
}
}

return delta_levels;
}

AmrTag CheckAllRefinement(MeshBlockData<Real> *rc, const AmrTag &level) {
// Check all refinement criteria and return the maximum recommended change in
// refinement level:
// delta_level = -1 => recommend derefinement
Expand All @@ -62,15 +93,19 @@ AmrTag CheckAllRefinement(MeshBlockData<Real> *rc) {
// neighboring blocks. Similarly for "do nothing"
PARTHENON_INSTRUMENT
MeshBlock *pmb = rc->GetBlockPointer();
// delta_level holds the max over all criteria. default to derefining.
AmrTag delta_level = AmrTag::derefine;
static const bool check_refine_mesh =
pmb->packages.Get("Refinement")->Param<bool>("check_refine_mesh");
// delta_level holds the max over all criteria. default to derefining, or level from
// MeshData check.
AmrTag delta_level = level;
for (auto &pkg : pmb->packages.AllPackages()) {
auto &desc = pkg.second;
delta_level = std::max(delta_level, desc->CheckRefinement(rc));
if (delta_level == AmrTag::refine) {
// since 1 is the max, we can return without having to look at anything else
return AmrTag::refine;
}
if (check_refine_mesh) continue;
// call parthenon criteria that were registered
for (auto &amr : desc->amr_criteria) {
// get the recommended change in refinement level from this criteria
Expand Down Expand Up @@ -150,9 +185,111 @@ AmrTag SecondDerivative(const AMRBounds &bnds, const ParArray3D<Real> &q,
return AmrTag::same;
}

void SetRefinement_(MeshBlockData<Real> *rc) {
void FirstDerivative(const AMRBounds &bnds, MeshData<Real> *mc, const std::string &field,
const int &idx, ParArray1D<AmrTag> &delta_levels,
const Real refine_criteria_, const Real derefine_criteria_,
const int max_level_) {
const auto desc =
MakePackDescriptor(mc->GetMeshPointer()->resolved_packages.get(), {field});
auto pack = desc.GetPack(mc);
const int ndim = mc->GetMeshPointer()->ndim;
const int nvars = pack.GetMaxNumberOfVars();

const Real refine_criteria = refine_criteria_;
const Real derefine_criteria = derefine_criteria_;
const int max_level = max_level_;
const int var = idx;
auto scatter_levels = delta_levels.ToScatterView<Kokkos::Experimental::ScatterMax>();
par_for_outer(
PARTHENON_AUTO_LABEL, 0, 0, 0, pack.GetNBlocks() - 1, bnds.ks, bnds.ke, bnds.js,
bnds.je,
KOKKOS_LAMBDA(team_mbr_t team_member, const int b, const int k, const int j) {
Real maxd = 0.;
par_reduce_inner(
inner_loop_pattern_ttr_tag, team_member, bnds.is, bnds.ie,
[&](const int i, Real &maxder) {
Real scale = std::abs(pack(b, var, k, j, i));
Real d = 0.5 *
std::abs((pack(b, var, k, j, i + 1) - pack(b, var, k, j, i - 1))) /
(scale + TINY_NUMBER);
maxder = (d > maxder ? d : maxder);
if (ndim > 1) {
d = 0.5 *
std::abs((pack(b, var, k, j + 1, i) - pack(b, var, k, j - 1, i))) /
(scale + TINY_NUMBER);
maxder = (d > maxder ? d : maxder);
}
if (ndim > 2) {
d = 0.5 *
std::abs((pack(b, var, k + 1, j, i) - pack(b, var, k - 1, j, i))) /
(scale + TINY_NUMBER);
maxder = (d > maxder ? d : maxder);
}
},
Kokkos::Max<Real>(maxd));
auto levels_access = scatter_levels.access();
auto flag = AmrTag::same;
if (maxd > refine_criteria && pack.GetLevel(b, 0, 0, 0) < max_level)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why has the max_level condition been introduced here (and for the other second deriv)?
Is this actually required (because it's also checked in SetRefinement IIRC.
And if it's needed, does this mean downstream code would also have to include this kind of check in custom refinement ops?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the SetRefinement check is for the gloabal max_level, where this is the parthenon/refinement#/max_level. Previously this was being done in the meshblock CheckAllRefinement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. That makes sense.

I'm still not sure this check should belong inside the kernel (rather than being a general feature of those refinement criteria derived from what's available within the refinement package).
Conceptually, I see the refinement critera itself as what's inside the kernel and the max level an outer constraint (though also being available on a per refinement criteria level).
So I see a danger if there are more refinement criteria are added the code for checking the max level would need to be duplicated and added to each kernel, rather than checking separately.

Having said that, I'm happy to fix this later so that this PR can go in quicker.

flag = AmrTag::refine;
if (maxd < derefine_criteria) flag = AmrTag::derefine;
levels_access(b).update(flag);
});
delta_levels.ContributeScatter(scatter_levels);
}

void SecondDerivative(const AMRBounds &bnds, MeshData<Real> *mc, const std::string &field,
const int &idx, ParArray1D<AmrTag> &delta_levels,
const Real refine_criteria_, const Real derefine_criteria_,
const int max_level_) {
const auto desc =
MakePackDescriptor(mc->GetMeshPointer()->resolved_packages.get(), {field});
auto pack = desc.GetPack(mc);
const int ndim = mc->GetMeshPointer()->ndim;
const int nvars = pack.GetMaxNumberOfVars();

const Real refine_criteria = refine_criteria_;
const Real derefine_criteria = derefine_criteria_;
const int max_level = max_level_;
const int var = idx;
auto scatter_levels = delta_levels.ToScatterView<Kokkos::Experimental::ScatterMax>();
par_for_outer(
PARTHENON_AUTO_LABEL, 0, 0, 0, pack.GetNBlocks() - 1, bnds.ks, bnds.ke, bnds.js,
bnds.je,
KOKKOS_LAMBDA(team_mbr_t team_member, const int b, const int k, const int j) {
Real maxd = 0.;
par_reduce_inner(
inner_loop_pattern_ttr_tag, team_member, bnds.is, bnds.ie,
[&](const int i, Real &maxder) {
Real aqt = std::abs(pack(b, var, k, j, i)) + TINY_NUMBER;
Real qavg = 0.5 * (pack(b, var, k, j, i + 1) + pack(b, var, k, j, i - 1));
Real d = std::abs(qavg - pack(b, var, k, j, i)) / (std::abs(qavg) + aqt);
maxder = (d > maxder ? d : maxder);
if (ndim > 1) {
qavg = 0.5 * (pack(b, var, k, j + 1, i) + pack(b, var, k, j - 1, i));
d = std::abs(qavg - pack(b, var, k, j, i)) / (std::abs(qavg) + aqt);
maxder = (d > maxder ? d : maxder);
}
if (ndim > 2) {
qavg = 0.5 * (pack(b, var, k + 1, j, i) + pack(b, var, k - 1, j, i));
d = std::abs(qavg - pack(b, var, k, j, i)) / (std::abs(qavg) + aqt);
maxder = (d > maxder ? d : maxder);
}
},
Kokkos::Max<Real>(maxd));
auto levels_access = scatter_levels.access();
auto flag = AmrTag::same;
if (maxd > refine_criteria && pack.GetLevel(b, 0, 0, 0) < max_level)
flag = AmrTag::refine;
if (maxd < derefine_criteria) flag = AmrTag::derefine;
levels_access(b).update(flag);
});
delta_levels.ContributeScatter(scatter_levels);
}

void SetRefinement_(MeshBlockData<Real> *rc,
const AmrTag &delta_level = AmrTag::derefine) {
auto pmb = rc->GetBlockPointer();
pmb->pmr->SetRefinement(CheckAllRefinement(rc));
pmb->pmr->SetRefinement(CheckAllRefinement(rc, delta_level));
}

template <>
Expand All @@ -165,8 +302,11 @@ TaskStatus Tag(MeshBlockData<Real> *rc) {
template <>
TaskStatus Tag(MeshData<Real> *rc) {
PARTHENON_INSTRUMENT
ParArray1D<AmrTag> delta_levels = CheckAllRefinement(rc);
auto delta_levels_h = delta_levels.GetHostMirrorAndCopy();

for (int i = 0; i < rc->NumBlocks(); i++) {
SetRefinement_(rc->GetBlockData(i).get());
SetRefinement_(rc->GetBlockData(i).get(), delta_levels_h(i));
}
return TaskStatus::complete;
}
Expand Down
Loading
Loading