search

21cmfast / 21cmFAST

Official repository for 21cmFAST: a code for generating fast simulations of the cosmological 21cm signal
MIT License
58 stars 37 forks source link

Improved implementation for perturbed density/velocity field #303

Closed BradGreig closed 1 year ago

BradGreig commented 1 year ago

This is a PR to fix an issue with the statistics of the perturbed density field as pointed out by @rajeshmondal18.

Previously, when calculating the perturbed density, all the mass of a perturbed cell was placed into a single cell. This features replaces this step with an implementation of cloud in cell to re-distribute the mass over the 8 neighbouring cells based on their distance from the centre of the perturbed cell.

This removes spiky features from the density field PDF and corresponding oscillatory features from the velocity PDF. The linear/log PDF of the density and velocity fields highlighting the issue are shown here while following this fix the correspond PDFs remove the sharp/oscillatory features.

Comparing the light-cones appears to show very little difference (see here). Reionisation histories and global signals remain unaffected. Looking at the 21-cm PS across the light-cone the largest differences occur at increasing z on the smallest scales. This difference is due to the fact that the previous implementation over estimated the small-scale power at increasing z. However, in all inference work, the thermal noise completely dominates at these scales and thus previous results should remain unaffected.

All test-data will need to be updated as a result of this fix.

I suspect this issue arose due to the PERTURB_ON_HIGH_RES option. The previous approach to just move all the mass into the new cell would work when perturbing directory onto the HII_DIM box. But since we now have the option to perturb directly onto the DIM box, the resolution difference would have amplified the effect.

steven-murray commented 1 year ago

Oh, I forgot to say that you need to update the CHANGELOG

steven-murray commented 1 year ago

Tests are failing due to a compilation error from a missing {, so that should be easy to fix.

BradGreig commented 1 year ago

I've made some updates to the code. I noted that the original usage of floats could cause issues on the periodic boundary conditions when re-distributing the mass over neighbouring cells. Upon replacement of this with doubles, the spike in the density PDF at overdensities of -1 has gone away as shown here here. Everything else has remained relatively unchanged.

Updating this double/float issue and correctly applying omp atomic statements has resolved almost all issues with the tests. One remaining test fail, which I haven't yet properly looked into. Though not immediately obvious why it is failing...

BradGreig commented 1 year ago

Hey @steven-murray I've established why that one test is failing. The relative tolerance is too high, causing it to fail. Changing rtol from the default 1e-5 to 1e-4 seems to alleviate the issue.

Under it's current state the main culprit is T_S, with 4961 cell fails. For T_k and x_e it's only 5 and 2.

In terms of an actual change to the numbers the failures on T_S are at the fourth/fifth decimal place. E.g. 26.2885 compared to 26.29 (this is one of the largest variants). In terms of a fractional change, this is < 0.01%.

Why this has now appeared I am not so sure. And I'm a little uncomfortable about it. Possibly the change to the density field may have amplified some rounding issues in the density field. And/or maybe there are larger variations in the density field owing to a broader distribution of possible densities. I'm not sure how much digging this requires, or whether it is comfortable enough just to raise the relative tolerance...

BradGreig commented 1 year ago

Hey @steven-murray @andreimesinger, in order to get the final test to pass I had to reduce the relative tolerance for that test data (see my comments above).

This PR now also includes a modification following #305. Changelog has been updated to reflect these changes.

codecov[bot] commented 1 year ago

Codecov Report

Base: 86.54% // Head: 86.53% // Decreases project coverage by -0.00% :warning:

Coverage data is based on head (98f81a7) compared to base (9e0a81b). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #303 +/- ## ========================================== - Coverage 86.54% 86.53% -0.01% ========================================== Files 13 12 -1 Lines 2809 2807 -2 ========================================== - Hits 2431 2429 -2 Misses 378 378 ``` Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=21cmfast). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=21cmfast)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

steven-murray commented 1 year ago

Thanks @BradGreig, and apologies for the slow response on this (I'm out at the Global 21cm Workshop this week). I'm a little uncomfortable with the tolerance change as well. I mean, the different is still very small, and it probably not a problem, but it bugs me that there's any difference at all. There shouldn't be essentially any randomness, right?

I think I'm happy for us to merge this, because it's not an urgent/significant problem, but I'd feel happier if we made an issue to remind ourselves to look into it a bit further.

andreimesinger commented 1 year ago

What is the hold-up for merging this?