The parameter, radiative_transfer_intermediate_step, should be True when calling solve_chemistry from EvolvePhotons and False when calling from MultiSpeciesHandler. Currently, this parameter will be set to True by EvolvePhotons, but not set to False for the MultiSpeciesHandler call. It also does not need to be written to/read from parameter file.
Update: edit description thanks to @aemerick's catch.
Thanks for this catch Britton. But in your description, don't you mean that radiative_transfer_intermediate_step is set to True in EvolvePhotons, but not set back to False in MultiSpeciesHandler?
I've been running simulations with this bug in place. Did you happen to notice if this bug cause any unusual or incorrect behavior?
Oops, yes, you are correct! I will edit the description to avoid confusion. I haven't actually run it without this fix, so I can't say how different things are. I'm about to run a test, so I can see.
hi @brittonsmith , where are we on this?
Just working on this right now. I'm actually finding some pretty significant differences between MultiSpecies and Grackle which may be a separate issue. Should have updates today or tomorrow.
Sorry for the delay. I have been struggling to find a conclusive test showing that this is working correctly. I've been working with a small cosmological simulation with radiating Pop III particles that @jwise77 provide me. Below are the results I'm finding. In each case, I show a temperature projection and phase plot at the same time for each run.
The phase plots aren't super helpful as there is some variation just do to stars forming, but the temperature plots clearly show that the ionization fronts get much further out in both Grackle cases than in the MultiSpecies case.
I've had a hard time testing this because differences in star formation cause things to diverge, but simpler tests show hardly any difference at all. Any advice on how better to look into this would be really appreciated.
I think a cosmology + galaxy simulation + RT has too much stochasticity to test a change like this. I would suggest using the RadiationTransport/PhotonTestAMR problem with the hydro turned off HydroMethod = -1 to test these different versions. It's a 1/r^2 density profile, so it has an analytical solution, which can be compared against.
I've been out of the loop on this for a bit, but checking in to see if there has been any more progress with this on a simpler test. I'm particularly interested since this directly affects some production runs I have run and am currently running.
My only insight is that the uniform box, single-star tests I did comparing Grackle+RT (before this PR) and MultiSpecies didn't show any obvious differences. Though in truth I did not do a rigorous comparison. Let me know if there is anything I can help with on this.
Sorry for the delay. I haven't had any time to get back to this recently and may not before the developers workshop.
@aemerick, I found very similar results in that simple tests with a single star particle looked very similar. I will see if I can find some time to look into @jwise's suggestion, but if I don't get to it, maybe this can be an activity for the workshop. It would be good to get this solved.
As a way to try and test how much this might affect simulations I've already done, I am running a production run with this bug fix for comparison. There are a lot of stochastic effects and randomness involved in my simulations, however. This will only be conclusive if the difference is truly dramatic (though likely not given the simple tests).
I'm sure we can figure this out during the workshop week if we don't get anything conclusive before then.
This PR passes the test suite at changeset 68c870a
I updated this PR as I found an additional issue with the radiation transfer parameters not being read in until after ReadParameterFile gets called. I've moved reading those parameters to RadiativeTransferReadParameters, which should solve that. However, this does mean that when Grackle prints out its runtime parameters after being initialized, they are not correct. The only clean solution I can see to that is to move initialization of all cooling functionality to a separate routine called after RadiativeTransferReadParameters. I've done this, but not included it yet as it's pretty invasive. Any thoughts on this are welcome.
This PR passes the test suite with no errors at changeset 166e23a
I'm currently setting up a PhotonTest test run with the MultiSpecies run, and before/after this PR.
As per my discussion with @jwise77 @brittonsmith @aemerick , everything agrees this is the correct thing to do and should be merged. I am going to approve and merge!