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 isotropic viscosity and resistivity #89

Merged
merged 68 commits into from
Sep 10, 2024
Merged

Add isotropic viscosity and resistivity #89

merged 68 commits into from
Sep 10, 2024

Conversation

pgrete
Copy link
Contributor

@pgrete pgrete commented Sep 29, 2023

What is says on the can: adds support for isotropic viscosity and resistivity

Description on usage and implementation part of the PR itself (so won't repeat here).

The artifacts from the new regression tests are for the viscosity
visc
and for the resistivity
ohm

Should be ready for review and comments.

@pgrete pgrete force-pushed the pgrete/diffusion branch 2 times, most recently from 35a3579 to 10281a1 Compare October 6, 2023 11:50
@forrestglines forrestglines self-requested a review January 27, 2024 23:13
@pgrete pgrete changed the base branch from pgrete/sts to main August 22, 2024 08:10
@pgrete pgrete changed the title Add isotropic viscosity Add isotropic viscosity and resistivity Aug 22, 2024
@pgrete pgrete requested review from bwoshea and removed request for forrestglines August 22, 2024 11:16
@pgrete pgrete mentioned this pull request Sep 9, 2024
Copy link

@bwoshea bwoshea 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 - I only have minor comments. Sorry it took me so long to go through it!

inputs/diffusion.in Outdated Show resolved Hide resolved
docs/input.md Outdated Show resolved Hide resolved
docs/input.md Outdated
<diffusion>
integrator = unsplit # alternatively: rkl2
#rkl2_max_dt_ratio = 100.0 # limits the ratio between the parabolic and hyperbolic timesteps
#cfl = 1.0 # Additional safety factor applied to diffusive timestep constraint. Default to hyperbolic cfl.
Copy link

Choose a reason for hiding this comment

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

again, for rkl2/operator split integrator (the main point here is that there are a lot of knobs to turn and it'd help to avoid confusion in this regard)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this applies to both. I adjusted the comment accordingly.

docs/input.md Outdated

conduction = anisotropic # none (disabled), or isotropic, or anisotropic
conduction_coeff = fixed # alternative: spitzer
thermal_diff_coeff_code = 0.01 # fixed coefficent in code units
Copy link

Choose a reason for hiding this comment

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

A general comment about coefficients - based on conversations with my current grad students, they would really appreciate if the units of the viscosity and resistivity were mentioned in the documentation, and perhaps a reference (even to a Shu or Spitzer textbook section) where they can figure out some details about the physics so they can set numbers appropriately.

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 just added the info on the units itself.
Regarding text book references, I'm happy if any student who does a deep dive intro opens a PR to extend the documentation accordingly.

docs/input.md Outdated
#### Viscosity/Momentum diffusion

Only isotropic viscosity with a (spatially and temporally) fixed coefficient in code units
is currently implemented.
Copy link

Choose a reason for hiding this comment

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

this would be a helpful place to mention the units of viscosity, even if it's only available in code units (this would help students who are trying to figure out what values are reasonable based on other simulation settings). Same thing applies for the resistivity subsection below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added info both here and in the overview block above.

src/hydro/diffusion/diffusion.cpp Show resolved Hide resolved
src/hydro/diffusion/resistivity.cpp Outdated Show resolved Hide resolved
src/hydro/diffusion/viscosity.cpp Outdated Show resolved Hide resolved
tst/regression/test_suites/diffusion/diffusion.py Outdated Show resolved Hide resolved
@pgrete pgrete enabled auto-merge (squash) September 10, 2024 12:17
@pgrete pgrete merged commit 7ea10bb into main Sep 10, 2024
4 checks passed
@pgrete pgrete deleted the pgrete/diffusion branch November 29, 2024 21:07
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