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

Is hydro diffusion doing the right thing in spherical coordinates? #171

Open
c-white opened this issue Oct 17, 2018 · 3 comments
Open

Is hydro diffusion doing the right thing in spherical coordinates? #171

c-white opened this issue Oct 17, 2018 · 3 comments
Labels
question Source code inquiries (not for asking about code usage) wontfix No action planned
Milestone

Comments

@c-white
Copy link
Contributor

c-white commented Oct 17, 2018

I was looking at the 3 locations in SphericalPolar::CoordSrcTerms() where hydro diffusion is accounted for. I don't know much about the diffusion implementation, but it struck me that some of the fluxes being used might not exist in certain cases, and at other times terms might not be accounted for.

The r-momentum is apparently affected by theta-fluxes of theta-momentum and phi-fluxes of phi-momentum:

if (do_hydro_diffusion) {
m_ii += 0.5*(phd->visflx[X2DIR](IM2,k,j+1,i)+phd->visflx[X2DIR](IM2,k,j,i));
m_ii += 0.5*(phd->visflx[X3DIR](IM3,k+1,j,i)+phd->visflx[X3DIR](IM3,k,j,i));
}

But in 1D or 2D will these always be set? Similarly for the theta-momentum:

if (do_hydro_diffusion)
m_pp += 0.5*(phd->visflx[X3DIR](IM3,k+1,j,i)+phd->visflx[X3DIR](IM3,k,j,i));

I suppose those arrays could just be 0 if the dimensionality is low enough, so the right thing is always done. I'm more confused by the phi-momentum source term:

if (use_x2_fluxes) {
u(IM3,k,j,i) -= dt*coord_src1_i_(i)*coord_src2_j_(j)*
(coord_area2_j_(j)*flux[X2DIR](IM3,k,j,i)
+ coord_area2_j_(j+1)*flux[X2DIR](IM3,k,j+1,i));
} else {
Real m_ph = prim(IDN,k,j,i) * prim(IM3,k,j,i) * prim(IM2,k,j,i);
if (MAGNETIC_FIELDS_ENABLED) {
m_ph -= bcc(IB3,k,j,i) * bcc(IB2,k,j,i);
}
if (do_hydro_diffusion)
m_ph += 0.5*(phd->visflx[X2DIR](IM3,k,j+1,i)+phd->visflx[X2DIR](IM3,k,j,i));
u(IM3,k,j,i) -= dt*coord_src1_i_(i)*coord_src3_j_(j)*m_ph;
}

Since use_x2_fluxes is true if and only if we are in 2D or 3D, it seems this term only gets added in 1D, in which case the referenced fluxes are 0 (going by the above logic). And it seems like the theta-phi viscous stress is not properly being accounted for in 2D and 3D.

Again, I'm not really familiar with how diffusion works in the code, so maybe this is all fine. But I'm working on a project that involves taking a very close look at curvilinear source terms, so I'm trying to understand exactly what's happening in this function.

@c-white c-white added the question Source code inquiries (not for asking about code usage) label Oct 17, 2018
@felker felker added this to the Future milestone Oct 17, 2018
@msbc msbc unassigned jmshi Nov 8, 2018
@stale
Copy link

stale bot commented Feb 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix No action planned label Feb 6, 2019
@stale stale bot closed this as completed Feb 13, 2019
@pdmullen
Copy link
Contributor

pdmullen commented Mar 16, 2021

Can we re-open this issue @felker? Stale-bot closed this on Feb 6, 2019; but @c-white raises some good questions here that were never addressed. I'd raise another one:

Remember that in the explicit integration of diffusive physics, flux contains the sum of standard (M)HD fluxes and diffusive fluxes. Do these geometric source terms accurately reflect this? This also shows up in cylindrical coordinates.

Aside: When producing the STS extension, I assumed that all geometric source terms (when diffusion is enabled) were correct; so if any geometric issues with explicit diffusion exist, they would have also been carried over into STS. My testing of curvilinear STS was limited in #299.

@felker felker reopened this Mar 16, 2021
@stale stale bot removed the wontfix No action planned label Mar 16, 2021
@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix No action planned label Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Source code inquiries (not for asking about code usage) wontfix No action planned
Projects
None yet
Development

No branches or pull requests

4 participants