Adding updates from other repositories to unigrid FLD solver, gFLDSplit

#216 Merged at 07e5479
Deleted repository
week-of-code (7fc9dc20617e)
  1. Daniel Reynolds

This PR includes updates I've been making to the split unigrid FLD solver in our large-scale version of Enzo.

New functionality: (1) split FLD solver can now interact with Enzo chemistry and cooling routines if desired, instead of always doing those internally, (2) updates to time stepping to enable more robust handling of new stars (and new radiation/ionization time scales), changing variable scalings, etc., (3) fixed FLD partial differential formulation to properly handle cosmological simulations starting at high redshifts, (4) updated default flux limiter behavior to better handle problem units and dynamics ranging a wider variety of problem scenarios, (5) added test problems for gFLDSplit + Enzo chemistry: all are in the run/RadiationTransportFLD directory, with names having the suffix "enzochem" (6) updated documentation for these new features, including both the Sphinx-based documentation of input parameters and the PDF-based documentation describing the equations and solvers.

With these updates, it's my recommendation that any interested users use gFLDSplit with Enzo's chemistry (instead of the other variants of the solver), since this formulation is the most robust and requires less knowledge of numerical solver details.

Comments (14)

  1. Sam Skillman

    Hi @drreynolds, this is a pretty extensive (and well documented) PR. Do you think it is worth adding .enzotest files for the new enzochem test problems? Since so many of your test scripts test against analytical answers, it is less clear that this is needed at the moment. I haven't had a chance to install HYPRE on the testing machine yet -- have you run the test_runner with the fld problems? If so, can you give me an estimate of how long the fld suite takes? FWIW, this passes the push test suite as it stands

    @jwise77 : could you take a read through the docs update?

    @devinsilvia : could you take a read through the src/enzo changes?

    I'll act as the third reviewer and try to run the fld tests. If someone else has run FLD lately and wants to verify some of these, that'd be great.

  2. Brian OShea

    Hi Dan,

    Chiming in here... I do think that you should add a set of "enzochem" FLD test problems to the test suite, replacing existing ones as necessary. I think that it's totally reasonable to update the gold standard based on improved physics capabilities, and given that we're working on fixes to the flux correction (that's mostly Dave C. and Greg B.), we'll likely have to regenerate the gold standard anyway.

    Also, regarding the test suite, all of the FLD enzotest files indicate that they have the FLD turned on using a parameter. I suspect that it should be possible to include/exclude any tests in the "quick", "push", or "full" suites that use FLD based on either some sort of user flag (default off, I suppose?) or some sort of simple test to see if the enzo binary includes the hypre library (maybe using otool on OS X or ldd on linux). Britton should probably chime in on that one, though!

  3. Brian OShea

    I'm not sure if the test runner can always distinguish how Enzo is configured, given that the binary doesn't have to be in the src/enzo directory - it can be anywhere. But, perhaps @brittonsmith has some thoughts on that. In the meantime, I'll look at the test runner and see if I can figure something out.

  4. Devin Silvia

    @samskillman I just finished going through the src/enzo changes and everything looks fine to me.

    In going through these files, I did notice a couple things that might be worth presenting to the dev community as a whole, which is defining a set of "Enzo Coding Standards". While there isn't anything worth delaying this PR for, there are cases where some of the variable names diverge from what I think many of us would consider to be standard Enzo variable names. For example there are cases where the unit conversation variables at things like dUnit and lUnit, rather than DensityUnits and LengthUnits. Also, there are cases of things like RhoNum instead of DensNum. I don't think these are unique to this PR and problems come from decisions made by the various contributors to the code base over the years. While converting all of the existing code to the same convention would be a big undertaking, it might be worth defining an enforcing some coding standard for future PRs, as I think it would simplify digesting source code. Since we have a more rigorous PR review system, enforcing such things should be easier.

    @drreynolds I noticed some removal of "RadHydroModel" from a couple of the problems but I couldn't tell if that would have a negative impact on anything. The parameter is still in the docs and used in some places, so I wanted to make sure this doesn't impact anything negatively. Also, have you tested that your changes still allow for either RT method to be used in the problems that should allow for it? It would be good to have confirmation that someone can switch from ray tracing to FLD and back without finding out that one of them is broken.

  5. Devin Silvia

    Hi Dan,

    That all sounds good to me. I don't see the need for time to be spent going through and changing any variable names for this PR. It mainly just got me thinking about the idea that Enzo doesn't have any sort of defined coding standards for developers to follow. While defining and enforcing such coding standards could potentially be a pain, it something that I think might be worth thinking about as it could help to increased the readability of the source and simplify the process of modifying the code for new developers.

    Overall, as far as I can tell, this PR is good to go!

  6. Brian OShea

    Hi folks,

    This passes the push suite and we've looked through the PR and are happy with it. Any reason that we shouldn't accept this PR?


    1. Sam Skillman

      We are still waiting on comments about the docs from @jwise77 and I still need to set up the automatic testing for FLD. I will try to do this today, but this should not be accepted (even though it looks great!) until both of those things happen.

  7. Daniel Reynolds author

    The PR has now been updated with the minor change I mentioned earlier this morning. I think it's ready for Sam to set up automatic testing of FLD.

    1. Sam Skillman

      After @drreynolds last update I re-upped the gold standard and now everything is passing. I heard from @jwise77 who said he'd be able to comment on the docs by early next week. I'm approving this now, but it shouldn't be accepted till after @jwise77 comments. @devinsilvia approve?

  8. Daniel Reynolds author

    I've updated one more internal FLD solver parameter, which will affect the gold standard FLD problems. If we could re-generate the gold standard when this PR passes the rest of the review process, it would be great.

  9. John Wise

    There are a couple of minor things in the documentation changes. 1. I found another typo, "gFLSplit" in the first sentence. 2. Regarding the parameter RadHydroMaxSubcycles, should this parameter always be 1 when using Enzo's chem/energy solver? If so, this should be explicitly stated in the parameter's description. What happens when the parameter is less than 1? Does it take multiple hydro timesteps for every FLD timestep?

    Other than that, everything LGTM, and it should be accepted.