PrincetonUniversity / athena

Athena++ radiation GRMHD code and adaptive mesh refinement (AMR) framework
https://www.athena-astro.app
BSD 3-Clause "New" or "Revised" License
235 stars 127 forks source link

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

Open c-white opened 6 years ago

c-white commented 6 years ago

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:

https://github.com/PrincetonUniversity/athena/blob/5fd9ea5424830daa33a444094a654f382b395781/src/coordinates/spherical_polar.cpp#L526-L529

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

https://github.com/PrincetonUniversity/athena/blob/5fd9ea5424830daa33a444094a654f382b395781/src/coordinates/spherical_polar.cpp#L554-L555

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:

https://github.com/PrincetonUniversity/athena/blob/5fd9ea5424830daa33a444094a654f382b395781/src/coordinates/spherical_polar.cpp#L560-L573

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.

stale[bot] commented 5 years ago

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.

pdmullen commented 3 years ago

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.

stale[bot] commented 2 years ago

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.