Skip to content
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

Merged
merged 19 commits into from
Feb 8, 2023
Merged

Updated version of mflike #53

merged 19 commits into from
Feb 8, 2023

Conversation

sgiardie
Copy link
Collaborator

@sgiardie sgiardie commented Jan 30, 2023

@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.

  • The new structure of the data block in MFLike.yaml is:

data:
experiments:
- exp1_freq1
- exp1_freq2
- ...
- exp2_freq1
- exp2_freq2 ...

vs the present structure:

data:
experiments:
exp1:
frequencies: [freq1, freq2, ...]
exp2:
frequencies: [freq1, freq2, ...]

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...

  • Also the _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 the top_hat_band block in MFLike.yaml:

top_hat_band:
nsteps: 1
bandwidth: 0

(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

  • Since we are not reading bandpasses from an external file, the obsolete functions regarding polarized arrays have been removed
  • In the case all bandpass_shift params are fixed to zero, the code computes band integration only once, avoiding a redundant operation at each step. Of course, possible band integration of foreground model would still be performed at each step
  • The calibration function has been modified in order to divide spectra by cal factors
  • The code now prints the number of bins used, to easily extract the number of degrees of freedom in the chi2 analysis
  • The user has now the possibility to set lmax_theory in the yaml file. If it is left to the default lmax_theory: null, the parameter is set in mflike to be 9000

This sould fix #50

@xgarrido xgarrido added the enhancement New feature or request label Jan 30, 2023
@xgarrido
Copy link
Collaborator

The unit tests fail because we forgot to update the notebook example. I'll do it asap.

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2023

Codecov Report

Merging #53 (d064697) into master (f271865) will increase coverage by 9.51%.
The diff coverage is 84.76%.

Additional details and impacted files

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
mflike/theoryforge_MFLike.py 84.78% <81.39%> (+17.49%) ⬆️
mflike/mflike.py 89.56% <100.00%> (+0.25%) ⬆️

Copy link
Collaborator Author

@sgiardie sgiardie left a 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

@xgarrido
Copy link
Collaborator

xgarrido commented Feb 2, 2023

Sorry @xgarrido, shouldn't we also add self.use_systematic_template = False here? 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 use_systemactic_template is not needed when retrieving foreground model.

@xgarrido
Copy link
Collaborator

xgarrido commented Feb 2, 2023

Actually we can't use the get_modified_theory function without mflike object (just to be clear, we need spec_meta and we can't provide a default value for this one)

@sgiardie
Copy link
Collaborator Author

sgiardie commented Feb 2, 2023

the tests fail because of the kernel I have used to execute the mflike notebook I think

Copy link
Member

@HTJense HTJense left a 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).

@sgiardie
Copy link
Collaborator Author

sgiardie commented Feb 6, 2023

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 use_top_hat_band: false or use_systematics_template: false in the yaml file, such that the corresponding blocks in the yaml can still be there uncommented but can be ignored if these two parameters are false

@xgarrido
Copy link
Collaborator

xgarrido commented Feb 6, 2023

Maybe it would be more linear to just add a use_top_hat_band: false or use_systematics_template: false in the yaml file, such that the corresponding blocks in the yaml can still be there uncommented but can be ignored if these two parameters are false

I'd prefer the null approach as suggested by @HTJense and add a commented example in order to not clutter the yaml file and not force the user to always set the true/false flags within the yaml file (as well as not providing default values for these flags). If the user sets a top_hat_band block then we can suppose he is going to use it. Otherwise the default is not to set it and then, not to use it. The theory extra_args is a pretty good example since, if the user does not set it, then the default is not to use it.

@xgarrido
Copy link
Collaborator

xgarrido commented Feb 6, 2023

@HTJense
Copy link
Member

HTJense commented Feb 6, 2023

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 get_requirements function to something like:

get_requirements(self):
    reqs = dict(Cl={k: max(c, self.lmax_theory + 1) for k, c in self.lcuts.items()})
    for k in self.expected_params_fg + self.expected_params_nuis: reqs[k] = None
    return reqs

I can't seem to (easily) fix this by changing the yaml setup without putting the parameter declarations in awkward locations.

@xgarrido
Copy link
Collaborator

xgarrido commented Feb 6, 2023

I'm not sure I understand your problem but it might be due to the fact that if you want to use mflike or any inherited class from mflike then you have to provide a default set of fg/nuisance parameters. In case of nuisance parameters then their names are related to the experiments block in the yaml. So you may face issue if you want to directly use mflike with ACT array name since there are no default value within mflike for parameters such as cal_dr6_pa4_f150. So to make things clear, I think we should not directly use mflike for ACT likelihood (https://github.com/ACTCollaboration/ACT-DR6-parameters/blob/main/likelihood_mflike_dr6.yaml#L1). What we have to do is to define an act_dr6 likelihood which inherits from mflike, something like

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 yaml file i.e. act_dr6.yaml which can look like

# 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 yaml file will look like

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 mflike with a bunch of yaml files whether we want to use it for SO, ACT or whatever

@HTJense
Copy link
Member

HTJense commented Feb 6, 2023

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 get_requirements function, as it can (in my experience) occasionally trip up cobaya if you don't properly pass on every parameter you want. For example if you have two likelihoods that both rely on the same parameter, cobaya might only pass on that parameter to one of the two likelihoods if they don't explicitly request it. This can realistically happen if you want to perform a run of say LAT + Planck with a (partially) identical foreground model.

@xgarrido
Copy link
Collaborator

xgarrido commented Feb 6, 2023

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).

The design here is basically the same as for Planck 2015/2018 likelihoods in cobaya's repository and the idea is to have a ACT DR6 likelihood python package with a proper setup.py file to install it and its dependencies.

What I really don't want is to have yaml files in mflike repository unrelated to SO science. The https://github.com/ACTCollaboration/ACT-DR6-parameters repository is one way and it's actually fine especially when running MCMC.

Either way, I do think the expected foreground & nuisance parameters should be passed on via the get_requirements function, as it can (in my experience) occasionally trip up cobaya if you don't properly pass on every parameter you want. For example if you have two likelihoods that both rely on the same parameter, cobaya might only pass on that parameter to one of the two likelihoods if they don't explicitly request it. This can realistically happen if you want to perform a run of say LAT + Planck with a (partially) identical foreground model.

If you do that then you provide default values to any fg/nuisance parameters (None value but still default value) and I would much prefer the code tells the user there is something wrong than providing a default set of values. I guess that's why cobaya has made this choice and asks likelihood developers to provide the correct set of model parameters.

Sharing same parameter is another problem and much like a foreground design issue. Actually having exactly the same parameter name should tell cobaya is the same parameter to sample (see A_planck for TT, TE, EE planck liklihoohds)

@sgiardie sgiardie merged commit 1ff870d into master Feb 8, 2023
@xgarrido xgarrido deleted the updated_mflike branch February 8, 2023 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expected calibration parameters causes trouble with multiple experiments
5 participants