Modifications for ACT DR6 analysis

sgiardie commented 9 months ago

I have done few modifications for ACT DR6 analysis, to make the yaml files more clear to the users and to remove hard coded parameters in the FG model.

I have moved the hard coded foreground parameters to the yaml (fixing them to the values they have in the current version of _get_foreground_model). The name of these new params can change, I just set them as placeholders now
To remove the calG_all and calT_{exp} from the yaml file (since they will be fixed to 1 in ACT DR6 analysis), I have removed them from the expected_params_nuis and fixed them to 1 in the initialization of TheoryForge. This way they can disappear from the yaml and I didn't have to modify _get_calibrated_spectra massively

sgiardie commented 9 months ago

Sorry, of course I could not fix them to 1 at the initialzation of TheoryForge. I could only add them back to the expected_params_nuis list at initialization and then fix them to 1 in the get_modified_theory function

sgiardie commented 9 months ago

Ok, change of plans: it is better not to modify the expected parameter list but to just allow parameters to be fixed to the correct value if they are not present in the nuisance params list (e.g. nuis_params.get("calG_all",1))

I also removed the polarization angles, for the same reason (they will not be used in ACT DR6 parameter analysis)

xgarrido commented 9 months ago

I'm fine with the foreground part (maybe adding some default latex name to the new parameters is worth it).

For the other changes, I think I do not really get the point of hiding parameters because ACT DR6 will not use them. It looks a bit strange to have within the LAT likelihood stuff specific to another experiment. I may understand that some parameters have default value but, as far as I understand cobaya's likelihood philosophy, all the parameters use by a likelihood should be clearly exhibit within its associated yaml configuration file. This way, external people can get what are the ingredients of the likelihood (and knowing that a value is fixed to 0 or 1 is already an important info.) and do not have to dig into the code to discover some hidden fudge factors.

@thibautlouis may have other comments regarding the terminology and the need for cal, calT or calE but, regarding the design choice, I'm really not convinced by this new hidden parameters.

sgiardie commented 9 months ago

Thanks for your comment @xgarrido. I agree that it is not ideal to have this configuration, and actually our current purpose (a part from pulling out the hard coded fg params) is to make the yaml file for the systematic params as clear as possible. So, avoiding all the parameters that would not be used for dr6 analysis (calT and polarization angles), such that the naive user should not wonder what they are since they would not appear in the dr6 paper. It also would not be ideal to directly remove all the params we don't use from the whole code (e.g., we still use polarization angles in SO analysis and cannot remove the associated parameters from the code). In my idea, we could keep these modifications in this branch and not merge it to the master. Of course, the dr6 specific likelihood that would inherit MFLike should refer to this branch version (if possible). I indeed opened this pull request so that we could discuss modifications more clearly but I am ok with not merging to the master

xgarrido commented 9 months ago

Having the stuff related to ACT DR6 within a dedicated branch is not the way to go, for different visibility reasons but also because it will be impossible to pip-install ACT DR6 likelihood.

I haven't tested it myself, but I think your modifications will make impossible to sample the calT, alpha parameters for ACT DR6 as well as for SO LAT (you can get parameters but there are not registered by cobaya as input parameters).

My point is that if we want to change things specific to ACT DR6, we should do it within the https://github.com/ACTCollaboration/act_dr6_mflike likelihood. So far the act_dr6_mflike is just an inherited class of mflike: it's basically empty and use everything implemented by mflike. But inheriting allow to overload any methods of mflike especially if we feel there are not suitable for ACT DR6 (at some point if we overload everything, one may consider the ACT DR6 likelihood as an independent likelihood with no relation with mflike; we are clearly not at this point). So to make my point I have created a PR https://github.com/ACTCollaboration/act_dr6_mflike/pull/6 that, I think, address your concerns. Btw, the question of slogdet can also be addressed in act_dr6_mflike fork by overloading the loglike method this way

    def loglike(self, cl, **params_values_nocosmo):
        ps_vec = self._get_power_spectra(cl, **params_values_nocosmo)
        delta = self.data_vec - ps_vec
        logp = -0.5 * (delta @ self.inv_cov @ delta)
        return logp

At the end of the day, if we think these changes should also benefit to mflike and to SO LAT, then we should push them to the upstream repository. But I still prefer to play in the ACT DR6 sandbox than changing/breaking things in mflike (with potential side-effects to SO LAT analysis) in order to propagate them to downstream likelihoods.

Again, regarding the new foreground parameters, I'm completely in favor of merging these things.

thibautlouis commented 9 months ago

my view is that we should fix the master version of mflike and produce a new version of the code. People can still use the old version if they prefer it. In the new version, we should remove the cst factor from the chi2 if we decide its irrelevant both for ACT and SO, and we should fix the way data are calibrated with a physical model. (cal_dipole, rel_cal_array, pol_eff_array). cal_dipole would be the uncertainties coming from planck overall calibration (called cal_Gall now), rel_cal_array would be the relative calibration of each array with respect to this, and pol_eff_array would be the physical polar efficiency. I think this is very straightforward to code and a better basis to rely on in the future.

sgiardie commented 8 months ago

I moved Xavier's solution allowing parameters not to be used in the yamls (only calT and alpha, we are using the total calibration now) in LAT_MFLike instead of the specific dr6 likelihood. This way, there will be no mention of these parameters in both dr6 specific yamls and code. This solution works fine with the master branch of the dr6 likelihood.

sgiardie commented 8 months ago

I merged the modifications done on the master to this branch. @xgarrido do you think it can be merged? this is related to https://github.com/ACTCollaboration/act_dr6_mflike/pull/7

simonsobs / LAT_MFLike

Modifications for ACT DR6 analysis #69