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

Fixed ACG flag when using FAST_FCOLL_TABLES #256

Closed JulianBMunoz closed 2 years ago

JulianBMunoz commented 2 years ago

It was only written for GaussLegendreQuad, but not for Nion_ConditionalM, and the latter was calling the former.

steven-murray commented 2 years ago

Hey @JulianBMunoz, I'm not sure I'm understanding what was wrong here?

JulianBMunoz commented 2 years ago

Hey @JulianBMunoz, I'm not sure I'm understanding what was wrong here?

The flag FAST_FCOLL_TABLES should only apply for MCGs (minihaloes) by default, since it works to good precision and improves the speed. For ACGs we found that it does not work as precisely and it barely saves any time. I added the global parameter global_params.USE_FAST_ATOMIC which is by default False unless the user really wants it on. I implemented it on the function GaussLegendreQuad_Nion (which computed the SFRD tables for ACGs in some range of deltas), but not on Nion_ConditionalM (which does the same but on a different set of deltas). As a consequence, I think the code was using the wrong function for ACGs when FAST_FCOLL_TABLES was on, which may matter a bit for low z.

I thought fixing would be as trivial as adding the && statement above, but somehow that makes all sorts of errors?

steven-murray commented 2 years ago

Ah, I see. That looks reasonable then. It is just the relvel integration tests that are failing (for coeval and lightcone). Does that help narrow it down?

BradGreig commented 2 years ago

Hey @JulianBMunoz, it looks like you might just need to update the test data for the specific model that @steven-murray mentioned (relvel).

Though, looking at the integration test data, the "fast_fcoll" data may also need to be updated (given the data will have changed for similar reasons). Likely it was lucky that that didn't fail too.

codecov[bot] commented 2 years ago

Codecov Report

Merging #256 (21db294) into master (efa47c5) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #256   +/-   ##
=======================================
  Coverage   86.23%   86.23%           
=======================================
  Files          12       12           
  Lines        2776     2776           
=======================================
  Hits         2394     2394           
  Misses        382      382           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c5a98d7...21db294. Read the comment docs.