New theory object with contaminants #24
Conversation
marlena6
left a comment
marlena6
left a comment
There was a problem hiding this comment.
Hi Andreu, I'm reading through and trying to understand the updates. Do your theory.py and model_lya.py replace lya_theory.py and lyaP3D.py? Or is the plan to make them all interact? I see some functions here with new functionality for contaminants, but some other parts seem to repeat existing functionality, so I'm wondering if the idea is I take your new files and integrate them with the old functions, or if you're planning to do that integration, or if this is written to supersede absolutely everything in lya_theory.py and lyaP3D.py? For example, I see an essentially empty get_default_igm_params in model_lya, which is existing functionality in lya_theory, so I guess you've left some things as dummy functions because you haven't yet integrated the existing code? But then it looks like you've rewritten the step of emulating the Lya params, although it's not clear to me why that couldn't have been done from the earlier code? Maybe this will be easier to clarify once we get a chance to talk in person; for now I'll keep working on testing the minimizer and fitting, etc, from my own branch.
|
Ok, looking through it more I guess a lot of this is still experimentation? If I'm reading it right, the model used to get the Lya + metals P3D only uses large scales, without Arinyo corrections, at the moment? Also, I see you've taken out most or all of the uses of the ForestFlow P3D function, is that purposeful or just for testing? |
|
At this point it is mostly experimentation, we can decide on Friday what the next steps should be. But yes, the two new modules would substitute the existing ones (note that forestflow_emu is not needed either). Before the discussion on Friday, however, it would be very useful if you had a look at the Appendix A in Overleaf, since writing that is what triggered me to take a shot at coding these up after seeing the the current implementation in the main branch had some limitations. |
|
Regarding whether to have or not the Arinyo model applied to the Lya x Silicon, this is a decision that we'll have to take at some point, but that it would be trivial to add to the code. The same applies to the Lya x HCD correlation, it is not clear whether this one should also have the Arinyo component to the Lya field, or just the Kaiser one. In the long term, the studies by Lucía and Daniel on HCD and metals will help us decide, but meanwhile we should just use one or the other (they should have almost no impact to the DR2 fits, I think). |
|
An example of limitations in the current implementation is that it was using P3D functions from ForestFlow, that didn't know about HCDs or metals. I did recycle the Px functionalities like PX_Mpc_detailed, but not the ArinyoModel since it wasn't very useful once you take into account contaminants. |
|
Regarding Px_Mpc_detailed, I might open an issue to make it even more basic, without p3d_params or new_cosmo_params variables that are now required, and without z. Really it should only require a function that returns |
…e, added back in colore defaults, added back in some functions to likelihood and minimizer
andreufont
left a comment
andreufont
left a comment
There was a problem hiding this comment.
I think this is now ready to merge. There are still a few functions to copy from "old" to "new" objects, but we can probably take care of these afterwards.
Before merging, though, it might be good to check that you can reproduce some of the old tests that you had (like krange and others), so that we are not breaking anything important... I'll be able to work on this next week, and I'd be happy to review / update old notebooks
|
Ok, I'll plan go through the old tests on Monday before we do the merge |
…place in notebook only, and one extra function in likelihood that optionally adds noise.
| optionally adding noise from the data cov | ||
| this is mostly useful for saving forecasts, otherwise one can just call get_convolved_px directly """ | ||
|
|
||
| cosmo = self.theory.get_cosmology() |
There was a problem hiding this comment.
For consistency, I believe we should pass params to get_cosmology here, in case we are generating a forecast for a cosmology different than the fiducial one in the theory class.
Alternatively, one could have an assert to check that none of the params are cosmological, but I think this is more work than just passing params here.
| # we may want to save the lya params in the output file | ||
| lya_params = self.theory.lya_model.get_lya_params(cosmo, params) | ||
| else: | ||
| lya_params = {} |
There was a problem hiding this comment.
Why are you storing the lya_params when working with "igm theories"?
We might want to create a forecast with an Arinyo model, and still save the parameters, right?
There was a problem hiding this comment.
How about other parameters used to make the forecast? For instance, do you want to store the value of contaminants? Or is the assumption that you'll never make forecasts with contaminants? If the latter, then I'd add an assert to make sure that we are not adding the contaminants (you can check the "include_" variables in the theory class).
|
Ready for merging (can do more profiling updates later) |
2 participants
Again, draft PR only to see the changes.
Not ready for public consumption yet.