Skip to content

Fargo#127

Open
adamdempsey90 wants to merge 17 commits into
developfrom
dempsey/fargo3
Open

Fargo#127
adamdempsey90 wants to merge 17 commits into
developfrom
dempsey/fargo3

Conversation

@adamdempsey90

Copy link
Copy Markdown
Collaborator

Background

This extends our linear advection machinery to work in spherical and cylindrical coordinates where the background velocity is assumed to be keplerian.

To keep the overlap integrals analytic, we make the approximation that in cylindrical/axisymmetric coordinates the radius in the keplerian Omega is the cylindrical R but in spherical is the spherical r.

This was done with the help of GPT (I still need to put the AI statement in the files).

This more or less works, but it's not quite ready yet.

Description of Changes

Checklist

  • New features are documented
  • Tests added for bug fixes and new features
  • (@lanl.gov employees) Update copyright on changed files
  • Any contribution that was created or modified with the assistance of generative AI must have a comment disclosing this such as // This file was created in part or in whole by generative AI

@adamdempsey90 adamdempsey90 requested a review from pdmullen May 12, 2026 19:27
… remaps in the X2 and X3 directions rather than try to reuse one routine but pass k-slices or j-slices.
Comment thread src/utils/diffusion/momentum_diffusion.hpp
Comment thread src/rotating_frame/params.yaml Outdated
@adamdempsey90 adamdempsey90 marked this pull request as ready for review May 27, 2026 20:09
@adamdempsey90

Copy link
Copy Markdown
Collaborator Author

@pdmullen tests now pass (I fixed one issue), so I think this is ready for real review.

Comment thread src/rotating_frame/rotating_frame.hpp Outdated
using ArtemisUtils::PLM;
using ArtemisUtils::VI;

#define CUB(x) ((x) * (x) * (x))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

CUBE


//----------------------------------------------------------------------------------------
//! template instantiations
template TaskListStatus Advect<Coordinates::cartesian>(Mesh *, const SimTime &);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Totally unrelated... but in the most recent parthenon swarm pack PR, I enrolled macros for template instantiations. We have these in almost every cpp file for different geometries... maybe we could remove a lot of lines of code by similarly adopting such a macro.

//! \fn Real RotatingFrame::OmegaKep
//! \brief Returns Keplerian angular velocity at spherical radius R
KOKKOS_FORCEINLINE_FUNCTION Real OmegaKep(const Real gm, const Real R) {
return std::sqrt(gm / (R * R * R));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

CUBE?

((GEOM == Coordinates::cartesian) || (GEOM == Coordinates::cylindrical)) ? 1 : 2;

KOKKOS_FORCEINLINE_FUNCTION Real OmegaKep(const Real R) const {
return std::sqrt(gm / (R * R * R));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

CUBE? Or just get get rid of CUBE?

Comment thread src/rotating_frame/rotating_frame.hpp Outdated
Comment on lines +346 to +347
if (j >= js && j <= je) v0(b, n, k, j, i) += dq / r.vol;
if (jp >= js && jp <= je) v0(b, n, k, jp, i) -= dq / rp.vol;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you help me understand the conditionals here?

Comment thread src/rotating_frame/rotating_frame.hpp Outdated
Comment on lines +365 to +366
if (k >= ks && k <= ke) v0(b, n, k, j, i) += dq / r.vol;
if (kp >= ks && kp <= ke) v0(b, n, kp, j, i) -= dq / rp.vol;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And here.

Comment thread src/rotating_frame/rotating_frame.hpp Outdated
oa.ApplyGradSkew(rd);
}
RemapUpdate(v0, ru, rc, vb, dwdt, three_d, b, n, k, j, j + joff, i);
RemapUpdateX2<GEOM>(oa, v0, ru, rc, vb, three_d, b, n, k, j, j + joff, i, jb.s,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also need some help here. Are we still doing a sweep as we had done it before. It looks like it, but more indexing enters the RemapUpdate function call.

// v_stored = v_full - v_bg(R)
// where v_bg = R*(OmegaKep(R) - omega_f) in the phi direction. The Riemann solver
// transports the stored momentum, so we must correct for the missing background
// angular momentum flux: d(rho*v_stored)/dt -= (1/V) * div(F_rho * v_bg).

@pdmullen pdmullen Jun 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I worry about consistency. Is the Riemann solve mass flux the right quantity to be used for the correction? We are inside an operator split update, it feels a bit odd to be using the mass flux computed from the final stage of e.g., an RK2 integration.

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.

2 participants