-
Notifications
You must be signed in to change notification settings - Fork 12
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
Updated version of mflike #53
Conversation
The unit tests fail because we forgot to update the notebook example. I'll do it asap. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #53 +/- ##
==========================================
+ Coverage 77.77% 87.29% +9.51%
==========================================
Files 3 3
Lines 405 370 -35
==========================================
+ Hits 315 323 +8
+ Misses 90 47 -43
|
…y forge When mflike is not provided, the intialization and setting part are delegated to other software such as pspipe
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.
Sorry @xgarrido, shouldn't we also add self.use_systematic_template = False
in the initialization of mflike (if mflike is None)? It is required in the get_modified_theory
function
We can. I have done the modifications with @adrien-laposta who uses only the theory forge part (no mflike intialization) for simulation purposes (and mainly for getting the foreground model). The |
Actually we can't use the |
the tests fail because of the kernel I have used to execute the mflike notebook I think |
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.
Looks good, my only comment is that the top_hat_band
block in MFLike.yaml is a bit weirdly implemented.
While the implementation works fine, it breaks a bit of a cobaya convention in the fact that normally, parameters are always present in the default yaml, and are supposed to be overridden in run settings, but in this implementation the parameter is not present by default and needs to be added manually to use it.
It's not wrong, it just breaks a bit of a convention and might make this particular setting awkward to find for an end user.
Perhaps either a default top_hat_band: null
parameter and if the user wants to use the top hat band they would fill out the block themselves (akin to the camb extra_args
block in cobaya).
Maybe it would be more linear to just add a |
I'd prefer the |
As a side remark, the |
In some cases (setups similar to this one ), I get an issue where MFLike does not receive the foreground/systematics parameters from cobaya. This is because MFLike only explicitly names the Cls are its requirements. This is fixed by changing the
I can't seem to (easily) fix this by changing the yaml setup without putting the parameter declarations in awkward locations. |
I'm not sure I understand your problem but it might be due to the fact that if you want to use from mflike import MFLike
class ACTDR6Like(MFLike):
"""
Likelihood for ACT DR6 data release (2022)
"""
file_base_name = "act_dr6"
install_options = {"download_url": f"/data/from/lambda/act_dr6.tar.gz"} and then associate to this likelihood a dedicated # Systematics
calG_all:
value: 1
latex: \mathrm{Cal}_{\rm G}^{\rm All}
calT_dr6_pa4_f150:
value: 1
latex: \mathrm{Cal}_{\rm T}^{\rm pa4 f150}
... Your running act_dr6_likelihood.ACTDR6Like:
data_folder: "/path/to/DR6/data"
... Maybe I miss the point but when this PR will be merged, I think this is the next step toward an ACT dr6 likelihood. Otherwise we are going to populate |
Ah, that would work as well. I did not realize that that was the design intention (I assumed mflike itself would be a general purpose likelihood, not a base class that specific likelihoods would inherit from). Either way, I do think the expected foreground & nuisance parameters should be passed on via the |
The design here is basically the same as for Planck 2015/2018 likelihoods in What I really don't want is to have
If you do that then you provide default values to any fg/nuisance parameters ( Sharing same parameter is another problem and much like a foreground design issue. Actually having exactly the same parameter name should tell |
@xgarrido and I have been updating mflike to allow the definition of systematic parameters per different experiments and frequencies (and remove obsolete functions). Indeed, the present version of mflike allows just the definition of systematic parameters per frequency, which is not enough when e.g. we have to deal with different experiments that share the same nominal frequencies.
data
block inMFLike.yaml
is:vs the present structure:
So basically the frequency layer has been removed and the freq information is encoded in the experiment list. This is also justified by the fact that the real frequency information would be extracted from the bandpasses, which we expect to be stored in the sacc file. So, the systematic parameters would be defined as
syst_exp1_freq1, syst_exp1_freq2
etc..._bandpass_construction
function has been changed: the default for bandpass construction is to read bandpasses from the sacc file. Band integration is automatically performed if the bandpass is multifrequency, not done otherwise. The option to read bandpasses from an external file has been removed. There is still the possibility to build simple top-hat bandpasses through thetop_hat_band
block inMFLike.yaml
:(which is a remaining of the
band_integration
block). For nsteps > 1 and bandwidth > 0, the top-hat bandpass would be built and band integration performed. The central frequency of the bandpass would be derived from the effective freq of the corresponding realistic bandpass in the sacc file, which follows the idea of extracting freq information from the info stored in the sacc. Instead, for nstep = 1 the code derives just a dirac delta function. This is useful in the case in which the bandpass in the code is a not dirac delta but the user wants to avoid band integration.The presence of this block in the yaml overwrites the default of using the bandpasses from the sacc file, so in the default case this block should be commented
lmax_theory
in the yaml file. If it is left to the defaultlmax_theory: null
, the parameter is set in mflike to be 9000This sould fix #50