Closed JulianBMunoz closed 1 year ago
@JulianBMunoz -- @BradGreig and I fixed a few things, and we think this is now ready. However, one thing is a little concerning. I was expecting the integration tests to fail, because changing TK should change the results of all the different tests -- however, not a single one failed, so the differences introduced are less than a factor of ~10^-3 in power spectrum and global signal. It's possible this is just due to the choices of parameters (and box size) in our tests, so it might be OK, but it would also be good if you could run a simulation (just a coeval) where you expect the difference to show, and post a plot showing the difference (maybe just run master branch with the same parameters to get it without the adiabatic fluctuations).
Patch coverage: 100.00
% and project coverage change: -0.09
:warning:
Comparison is base (
f98e698
) 86.50% compared to head (0ce7c92
) 86.42%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
I ran two lightcones: one with and one without the adiabatic fluctuations:
This looks a lot like the power spectra in @JulianBMunoz 's paper, so I think I'm happy to go ahead with this. Since this new fix is on by default, I'll have to re-run all the test data.
@BradGreig note that there was a bug fix in this PR that affects other stuff. Namely, the spin_temp
function was determining the "first box" by checking if the previous spin temperature was an instance of IonizedBox
instead of TsBox
. I had already flagged this in a comment as being weird, but never fixed it. It turns out that was causing the issue with the evolved boxes not containing the initial perturbations.
@steven-murray is this a 21-cm PS? Or something else? What has changed from the other week where you were finding no difference?
@BradGreig it was the bug in spin_temperature
: there was a check for whether the previous_spin_temp
was an instance of IonizedBox
which should have been TsBox
. Since it didn't find that, it overwrote the initial conditions.
@BradGreig I've addressed all your comments (there was one comment that might require a little more discussion). As to your main concern, about the API compatibility, I don't think this breaks the API (at least not significantly).
Old files should still be read in completely fine because the first_box
attribute in those files should just be ignored. I tried this by writing out an old IonizedBox
file with master
branch, then reading it in with this branch, and it reads fine.
There is a small API breakage in that if a user was directly accessing the first_box
attribute, it no longer exists, so their code will break. I doubt anyone is doing this though. Probably, this should correspond to a next minor version (not patch).
Hi @steven-murray, I've gone and clarified my meaning over one point (and agree with the initialisation of the tables). Hopefully that addresses the comment with more discussion that you refer to?
Thanks for the explanation on the first_box
, I didn't do a deep dive to see how relevant it would be. I was just thinking of the case that if the first_box
property was used for some reason. Although I completely agree that it is extremely unlikely and should be fine.
@BradGreig this is now passing tests and I think it's ready to go. Go ahead and Approve + Merge if you agree!
The assumption in 21cmFAST so far has been to fix the temperature Tk to be homogeneous at whatever initial z we take. This leads to an underprediction of adiabatic fluctuations even during cosmic dawn (2302.08506). Let's change to initializing the Tk box with linear adiabatic fluctuations given by the fitting formula in 2302.08506