While the roots of both projects are related, this is a complete new implementation from scratch. The main difference is that the model described (and implemented) in Schmidt & Federrath (2011) is a pure hydro model based on grid-scale quantities with a dynamical equation for the kinetic SGS energy, this implementation features hydro and MHD with the possibility of explicit filtering (i.e., resolved quantities on scales larger than the grid scales) and instantaneous kinetic and magnetic SGS energies, i.e., there's no evolution equations for the SGS energy (and therefore no intermediate reservoir that could be used for feedback purposes) but SGS energies are calculated "ad-hoc".
Having said that, if anyone is interested in a dynamical MHD SGS framework (e.g., for use with feedback mechanisms in simulations) I'd be happy to get in touch.
Would you mind doing the following?
updating run/MHD/3D/StochasticForcing/StochasticForcing.enzo so that it only goes for 3 dynamical times (and thus only runs for a minute or so on a modern-ish laptop)
modifying run/MHD/3D/StochasticForcing/StochasticForcing.enzotest so that pushsuite = True ?
Done (also updated the docs to the new format). The test run now takes ~70 sec on a single i5-7200U core.
What are other people's opinions on Nathan's comment? What's the preferred way of taking care of the typedefs I introduce? I personally have no strong preference.
I would agree with Nathan's comment because the variable names are so generic. I would preface them with SGS or the like.
@bwoshea and @ngoldbaum : Do you think this PR is ready for approval after passing the test suite? Any more review comments for @pgrete ?
Sorry for the delay in reply, @jwise77 . I ran the test suite, and this passes all pre-existing tests (with the new SGS test failing because there’s no pre-existing answers from the gold standard, which is to be expected). I’ll make one more pass through the code, but I think it’s ready to go.
I updated the PR in order to make the vector/tensor index variable names unique/less general, e.g., XX ->`SGSXX etc.
@pgrete , I’ve made some small notes below but overall this looks quite good to me.
I just addressed the last comments and updated to the current tip.
@pgrete , I looked over the code again and it looks good to go! @jwise77 and @ngoldbaum , what do you think?
I just updated the PR to include the latest comments:
Updated SGS function names
Fixed indentation and extra whitespace in SGS SourceTerms
Fixed inded typo SGS JacB typo in Grid destructor.
Changed printing debug messages for SGS to verbose debug1 mode
The last open item (for discussion) is probably whether the memory allocation should happen where it is right now or in the grid constructor, see my comment below.
I found your comment compelling; that is to say, no allocations take place in the Grid constructor by many other routines, so it would actually be going against current practice in Enzo to allocate your memory in the constructor. If you did so, it’d end up being more confusing in the long run. So, I would say leave it as it is, because the alternative is fixing an awful lot of other code, which is outside the scope of this PR.
I just pushed the hopefully final update in which I addressed the merge conflict due to recent merged PRs and made the SGS array handling in the Grid constructor/destructor conditional as per Britton’s suggestion.