-
Notifications
You must be signed in to change notification settings - Fork 15
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 thermal conduction and RKL2 supertimestepping #1
Conversation
…or using prolong/restric with non-base containers)
It looks like this also adds a new reconstruction method. Would it make sense to split that off into its own PR? or is that needed for the test problems here? |
That recon is already in |
@forrestglines @BenWibking I just fixed the tests and updated to latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed this as best I can, didn't find any obvious errors. If convergence looks good enough compared to literature, I think this is fine to merge
pkg->AddParam<>("thermal_diff", thermal_diff); | ||
|
||
const auto mu = pkg->Param<Real>("mu"); | ||
conduction_sat_prefac = 6.86 * std::sqrt(mu) * conduction_sat_phi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this 6.86 a magic number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it assumes a fully ionized hydrogen plasma. I added an appropriate comment to clarify.
if (parthenon::Globals::my_rank == 0) { | ||
const auto ratio = 2.0 * tau / mindt_diff; | ||
std::cout << "STS ratio: " << ratio << " Taking " << s_rkl << " steps." << std::endl; | ||
if (ratio > 400.1) { | ||
std::cout << "WARNING: ratio is > 400. Proceed at own risk." << std::endl; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we always want to output this? Writing to STDOUT every cycle? Could this cause a slow down on some machines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this useful when running simulations (as it explains/is related to the performance per cycle).
Given that std:cout is buffered I also don't expect any significant slowdown.
return TaskStatus::complete; | ||
} | ||
|
||
// Assumes that prim and cons are in sync initially. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this assumption affected by other user/pgen source terms that might be added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently not as it's both the very first task called before any other sources and the very last task called after FillDerived.
I added notes that if the position of the tasks changes, this should be updated.
@@ -2,5 +2,5 @@ | |||
|
|||
Executes 2D ring diffusion problem following Sharma & Hammett (2007) and calculates convergence rate. | |||
Errors are calculated based on comparison to steady state solution. | |||
Convergence for this problem is not great, but matches other numbers reported in literate, e.g., Balsara, Tilley & Howk MNRAS (2008) doi:10.1111/j.1365-2966.2008.13085.x . | |||
Convergence for this problem is not great, but matches other numbers reported in literature, e.g., Balsara, Tilley & Howk MNRAS (2008) doi:10.1111/j.1365-2966.2008.13085.x . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an image of the problem to share? Any obvious issues with AthenaPK's solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New features
Open items