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

fix: negative sign on adiabatic fluctuations #335

Closed steven-murray closed 1 year ago

steven-murray commented 1 year ago

Fixes an error where positive should be negative.

BradGreig commented 1 year ago

Hey @steven-murray, I just had a quick look at the failed tests, and I agree that it isn't immediately obvious what the issue is. The specific test is one where the explicit calculation is used rather than the interpolation tables, so theoretically differences could be coming in there (with numerical issues arising at extreme ends across platforms). Given this isn't replicatable locally that makes it difficult to isolate the source.

Can you provide a power spectrum plot of all the fields? Is that possible to pull from actions? (since locally you have no problems)

Looking at the data itself, it is not failing too greatly. It's of order ~2% where the relative tolerance is set to be ~1%. You could always bump it to just get it through for now. And just push the whole issue further down the road.

Although this shouldn't make any difference whatsoever, the variable growthfac should be declared in the shared variables (you can also remove inverse_growth_factor_z and growth_factor_zp since they are compressed into the above variable).

steven-murray commented 1 year ago

@BradGreig thanks for looking at this -- I'm pulling my hair out on this one!

I managed to upload plots as artifacts, so we'll have them from now on in all runs (go to Actions -> Tests -> Artifacts).

Here's the Ubuntu (failing) test: tests.test_integration_features.py--test_power_spectra_lightcone[mdz_tsfluct_nthreads].pdf

And here's the MacOS (passing) test:

tests.test_integration_features.py--test_power_spectra_lightcone[mdz_tsfluct_nthreads].pdf

BradGreig commented 1 year ago

Hey @steven-murray, yeah, this sounds eerily familiar to some of the issues I had on a previous test/PR. Looking at the PS plots, the most obvious issue is with the spin temperature box. It's not immediately obvious where that issue might be occurring though without a deep dive into all its components. However, this is impossible if it cannot be replicated locally.

As I mentioned above, I'd be fine with decreasing the tolerance of this test/all tests to see if it passes (from 0.01 to 0.02 hopefully would suffice). Decreasing the tolerance shouldn't really cause any issues. Any significant error etc. that is made or change in physics should result in much larger differences.

I know it is far from ideal to just reduce the tolerance (as we've done it previously). But maybe we just do it, flag it as an issue and further down the line we can return to this.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -0.02 :warning:

Comparison is base (55d2d37) 86.60% compared to head (37075a6) 86.58%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #335 +/- ## ========================================== - Coverage 86.60% 86.58% -0.02% ========================================== Files 12 12 Lines 2814 2811 -3 ========================================== - Hits 2437 2434 -3 Misses 377 377 ``` | [Impacted Files](https://app.codecov.io/gh/21cmfast/21cmFAST/pull/335?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=21cmfast) | Coverage Δ | | |---|---|---| | [src/py21cmfast/\_utils.py](https://app.codecov.io/gh/21cmfast/21cmFAST/pull/335?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=21cmfast#diff-c3JjL3B5MjFjbWZhc3QvX3V0aWxzLnB5) | `88.85% <ø> (-0.05%)` | :arrow_down: |

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