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 tracer particles to phoebus #190

Merged
merged 45 commits into from
Feb 16, 2024
Merged

Add tracer particles to phoebus #190

merged 45 commits into from
Feb 16, 2024

Conversation

AstroBarker
Copy link
Collaborator

@AstroBarker AstroBarker commented Dec 6, 2023

I believe that the tracer treatment is now mature enough to have more eyes on it. It's a pretty big PR and has been awhile coming, so interact in whatever level you feel.

Summary

Here I add tracer particle support in phoebus. We create a tracer package that, if enabled, registers the appropriate swarm variables. See src/tracers/tracers.cpp. At present, the tracers are coupled in first order operator split, though in the future I'd like to explore improving this, after some phoebus task list refactoring. Tracers are advected in AdvectTracers(...), interpolating the appropriate quantities to the tracer positions and then pushing (with RK2). The "filling" of the tracers is handled in FillTracers registered with UserWorkBeforeOutput. To enable this, I added phoebus::UserWorkBeforeOutput to the driver.
It's a lot of quantities.. and may desire to shorten later, or otherwise make what is stored runtime modular?

To deal with black hole accretion, when FMKS geometry is enabled, we use PurgeParticles in src/fixup/fixup_particles.cpp If FMKS is not enabled, the constexpr if resolves to a no op. In principle, here, future "purge particles if.." conditions can be supplied.

At present, I have implemented tracers in the torus and advection pgens. The advection test yields relative errors on tracer positions of ~ 1e-14 after one period. Tracer orbits in a non-magnetized torus work as desired.

To run with tracers (when the pgen supports it), simply

<physics>
tracers = true

<tracers>
num_tracers = ...

TODO:

  • Add tracers into advection, at least. Good to have a simpler-than-torus test problem.
  • Do 3D, non-magnetized torus orbit test.
  • Unit & regression test.
image

cc: @bprather

@AstroBarker AstroBarker added the enhancement New feature or request label Dec 6, 2023
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Overall this looks great! Thanks for your hard work @AstroBarker this is awesome. A few nitpicks below. And also tests seem to be not passing.

@@ -93,6 +93,7 @@ hydro = true
he = false
3t = false
rad = false
tracers = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you need to add this to all of the pgens if the default is false?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was using GetBoolean not GetOrAddBoolean(_, _, false). Fixed!


auto ib = pmb->cellbounds.GetBoundsI(IndexDomain::interior);
auto jb = pmb->cellbounds.GetBoundsJ(IndexDomain::interior);
auto kb = pmb->cellbounds.GetBoundsK(IndexDomain::interior);

PackIndexMap imap;
auto v = rc->PackVariables({fluid_prim::bfield}, imap);
std::vector<std::string> vars = {fluid_prim::bfield, fluid_prim::density};
Copy link
Collaborator

Choose a reason for hiding this comment

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

note that in #180 this syntax will change slightly. Should be easy to fix though.

Comment on lines 593 to 594
// TODO (BLB): better way to construct & access indices array?
// Access as indices( flatten( k-kb.s, j-jb.s, i-ib.s ) )
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like @brryan 's idea of using a "volume-weighted" mask, as discussed on MM. We should look at the tracers example parthenon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note we don't have to fix this now. PR can go in even if the initialization is rough around the edges.

src/tracers/tracers.cpp Show resolved Hide resolved
@AstroBarker
Copy link
Collaborator Author

re: tests failing, I had some old testing code in radiation_advection and radiation_equilibration problems that didn't work with tracers disabled. I've just reverted those files to pre-tracers, and will put tracers in the fluid advection problem.

// TODO (BLB): better way to construct & access indices array?
// Access as indices( flatten( k-kb.s, j-jb.s, i-ib.s ) )
const int mysize = (kb.e - kb.s + 1) * (jb.e - jb.s + 1) * (ib.e - ib.s + 1);
ParArray1D<int> indices("map from k,j,i to 1D particle array", mysize);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Weirdness on GPU accessing the indices gymnastics @Yurlungur . In Phoebus::ProblemGenerator::Torus::SumDiskCells kernel, n_cells_disk is always 0.

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 see the problem. indices is not guarded with an atomic. You need a Kokkos::atomic on the indices assignment. Alternatively, you need to make it a raw kokkos view with the atomic trait.

Real uphi;
if (r > rin) lnh = log_enthalpy(r, th, a, rin, angular_mom, uphi);
if (lnh > 0.0 && x1 > xh) {
indices(flatten(k - kb.s, j - jb.s, i - ib.s)) = n_cells_disk;
Copy link
Collaborator

Choose a reason for hiding this comment

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

n_cells_disk is not guaranteed to be assigned in any particular order... so making indices have atomic writes may not solve this problem. But let's try that first.

@AstroBarker
Copy link
Collaborator Author

Some updates:

  • I have refactored the torus pgen to use rejection sampling. It is cleaner now, and doesn't fail on GPU. As implemented, it should be uniform in volume.
  • I have split the tracer rhs calculation into a separate function
  • Tracer integration is now RK2, as in nubhlight
  • Added Bernoulli parameter - u^0 h - 1 to tracer output.
  • Tracers are implemented into the advection pgen
    • Implementation here could be improved. As is, we get nblocks * num_tracers particles. Should just move to post init...

In the advection test, relative errors on tracer positions was ~ 1e-14 or lower after one period.
I also ran a 3D, non-magnetized torus to see tracer orbits (notes: below only goes to t=500.. longer is running. And this was before the upgrade to RK2)

tr_eq

@Yurlungur
Copy link
Collaborator

Looks awesome! When do you think you'll be ready to remove the WIP markings? :)

@AstroBarker AstroBarker changed the title WIP: Add tracer particles to phoebus Add tracer particles to phoebus Jan 22, 2024
@AstroBarker
Copy link
Collaborator Author

One last change: I've moved the FillTracers call from PostFillDerivedBlock into UserWorkBeforeOutput. To accommodate this, I created phoebus::UserWorkBeforeOutput in the driver and registered it in main. At present it just unpacks MeshBlockData and calls FillTracers, but it could accommodate extension in the future. Should cut down on unnecessary work. Let me know how this bit looks @Yurlungur.

(Also fixed something I forgot to commit in the torus init..).

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

This looks great, @AstroBarker ! I'm happy to merge this, but I would like @brryan 's eyes on it if he gets the chance to look.

Also this is a question for Ben: Are particle IDs integers? Or longs? We might want to make them longs if possible (which might be a parthenon change). I recall hitting integer overflow in nubhlight.

src/tracers/tracers.cpp Show resolved Hide resolved
Comment on lines +1034 to +1041
void UserWorkBeforeOutput(MeshBlock *pmb, ParameterInput *pin) {
auto tracer_pkg = pmb->packages.Get("tracers");
bool do_tracers = tracer_pkg->Param<bool>("active");
if (do_tracers) {
auto &mbd = pmb->meshblock_data.Get();
tracers::FillTracers(mbd.get());
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 When particle meshblock packs work, we should refactor to use those :)

Real uphi;
if (r > rin) lnh = log_enthalpy(r, th, a, rin, angular_mom, uphi);
if (lnh > 0.0) {
const Real vol = coords.CellVolume(k, j, i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should have a factor of the metric in it I think. (It probably doesn't matter for problems we run, but we should include it.)

if (r > rin) lnh = log_enthalpy(r, th, a, rin, angular_mom, uphi);
if (lnh > 0.0 && x1 > xh) {
const Real vol = coords.CellVolume(k, j, i);
number_block_reduce += vol;
Copy link
Collaborator

Choose a reason for hiding this comment

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

factor of metric determinant missing (see above)

if (swarm_d.IsActive(n)) {
int k, j, i;
swarm_d.Xtoijk(x(n), y(n), z(n), i, j, k);
const Real vol = coords.CellVolume(k, j, i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing detgamma

Comment on lines +144 to +152
// sample in ye ball
Real r2 = 1.0 + rin * rin; // init > rin^2
while (r2 > rin * rin) {
x(n) = x_min + rng_gen.drand() * (x_max - x_min);
y(n) = y_min + rng_gen.drand() * (y_max - y_min);
z(n) = z_min + rng_gen.drand() * (z_max - z_min);
r2 = x(n) * x(n) + y(n) * y(n) + z(n) * z(n);
}
id(n) = num_tracers_total * gid + n;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@Yurlungur
Copy link
Collaborator

Actually @AstroBarker before merge, please correct the missing metric factor in the volumes in the torus calculation.

@Yurlungur
Copy link
Collaborator

Since @brryan never had a chance to look. I'm going to click the button. Ben whenever you get the chance, I'd still like your thoughts.

@Yurlungur Yurlungur merged commit 4e3d2c7 into main Feb 16, 2024
2 checks passed
@Yurlungur Yurlungur deleted the blb/tracers branch February 16, 2024 00:53
@Yurlungur
Copy link
Collaborator

Also thanks for all your hard work on this @AstroBarker !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants