-
Notifications
You must be signed in to change notification settings - Fork 47
Beta esc #538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Beta esc #538
Conversation
daviesje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming from your issue that you have already spoken with @andreimesinger or @steven-murray about the pros/cons of adding new parameters to the escape fraction so I'm just going to review the code here.
As for the "photon-conservation" model, I don't immediately see a problem with running our calibration simulation with a BETA_ESC in any of the cases, but maybe you found something wrong with the f-photoncons model here? In any case I wouldn't want these modules getting in the way of new features so in the worst case we can simply throw parameter errors when a user enables both
src/py21cmfast/src/hmf.c
Outdated
| double Zesc = log(pow((1 + p.redshift) / 8.0, p.beta_esc)); | ||
|
|
||
| return exp(Fstar + Fesc - p.Mturn_acg / exp(lnM) + lnM); | ||
| return exp(Fstar + Fesc + Zesc - p.Mturn_acg / exp(lnM) + lnM); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are very hot functions. So saving log/pow calls here is very important. You can see how the above function log_scaling_PL_limit does this for the other power-laws and you could use two less here. However more importantly, this term is only calculated once per redshift, so it can be saved as a constant in the scaling parameters and simply loaded in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I see that I actually double-counted this factor twice which is wrong (I already added this factor in Nion_ConditionalM). Sorry the code structure is too complicated for me to understand rn so this is probably stupid: Should I add it at prefactor_nion level in set_fixed_grids and get_box_averages? Or should I add this in set_scaling_constants level by directly changing f_esc10? Or somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I would implement this in the following way:
- Add a new
doublefield to theScalingConstantsstruct (inscaling_relations.h) which contains the entire power-law term fromBETA_ESC - Add a line to
set_scaling_constants (inscaling_relations.c) which initialises this term based on the redshift, this calculates the term once per redshift and stores it for fast multiplication later (no morepow` calls) - Multiply by this factor in two places: in
HaloBox.cwhere the escape fraction is set inset_halo_properties, and inIonisationBox.cwhere the constantion_eff_factor_glis set inset_ionbox_constants.
Regarding the last step, the reason you have to alter two parts is just due to the way the discrete halo / density field models are split. It's not ideal but that's the way it's organised at the moment. Happy to answer any further questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the detailed instruction, please let me know if the latest commit is doing things correctly. My only addition to your comment is that I think the prefactor_nion in set_fixed_grids and get_box_averages needs to be modified as well, otherwise the sampling below the mass limit when AVG_BELOW_SAMPLER will not have the correct scaling
src/py21cmfast/src/hmf.c
Outdated
| return IntegratedNdM(lnM_Min, lnM_Max, params, &u_nion_integrand, 0); | ||
| // z factor can be taken out of the integral | ||
| double Zesc = pow((1 + z) / 8.0, sc->beta_esc); | ||
| return Zesc * IntegratedNdM(lnM_Min, lnM_Max, params, &u_nion_integrand, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment, redshift-based escape fractions should be pre-computed once per snapshot
src/py21cmfast/src/hmf.c
Outdated
| if (params.HMF != 0 && params.HMF != 1 && params.HMF != 4) params.HMF = 0; | ||
| return IntegratedNdM(lnM1, lnM2, params, &c_nion_integrand, method); | ||
| // z factor can be taken out of the integral | ||
| return Zesc * IntegratedNdM(lnM1, lnM2, params, &c_nion_integrand, method); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well.
| raise ValueError( | ||
| "USE_MINI_HALOS is not compatible with the redshift-based" | ||
| " photon conservation corrections (PHOTON_CONS_TYPE=='z_photoncons')! " | ||
| " photon conservation corrections (PHOTON_CONS_TYPE=='z-photoncons')! " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!
| "This changes the escape fraction so it is not consistent with the manual setting of scaling." | ||
| "Set PHOTON_CONS_TYPE to 'no-photoncons' or 'alpha-photoncons' if you want the scaling to be exact.", | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone else may have an opinion on this, but if there is a real problem using these two models together then perhaps we should throw an error instead of a warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an error will be better. Warnings are too easy to ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand is what is happening when we turn on use_photoncons, but I see in the code f_esc10 gets modified per scaling relation if PHOTON_CONS_TYPE == 3. I will come back to it later when I understand the code better.
src/py21cmfast/wrapper/inputs.py
Outdated
| BETA_ESC = field( | ||
| default=0.0, | ||
| converter=float, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to have a type!
| BETA_ESC = field( | |
| default=0.0, | |
| converter=float, | |
| ) | |
| BETA_ESC: float = field( | |
| default=0.0, | |
| converter=float, | |
| ) |
daviesje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking a lot better, but I still have two questions:
- I know the Qin+25 model did not include minihalos at all, but would you like to include them? Either with the same scaling or another parameter? At the moment there is no scaling applied to them at all, and that might be confusing. If you wish not to include them, make it clear in the docstring for
BETA_ESC - Did you try any runs with
PHOTON_CONS=z-photoncons? I can't see anything that could go wrong here, sinceBETA_ESCshould be applied to both the calibration and followup simulations. The warning/error check might not be necessary
|
@zhaotingchen do you have updates to @daviesje's comments above? |

Attempt to fix #537