search

igmhub / cup1d

Cosmology using P1D - small scale clustering of the Lyman alpha forest
2 stars 1 forks source link

likelihood can not be setup with metals and nigm=2 (when also using highres data) #65

Closed andreufont closed 2 weeks ago

andreufont commented 2 months ago

The way we are dealing with the extra theory for high-res P1D is getting messy, I should take a look at this.

File /global/cfs/cdirs/desicollab/users/font/Codes/cup1d/cup1d/likelihood/likelihood.py:66, in Likelihood.__init__(self, data, theory, emulator, cosmo_fid_label, free_param_names, free_param_limits, verbose, prior_Gauss_rms, kmin_kms, emu_cov_factor, extra_p1d_data, min_log_like)
     63 self.theory = theory
     65 # setup parameters
---> 66 self.set_free_parameters(free_param_names, free_param_limits)
     67 if verbose:
     68     print(len(self.free_params), "free parameters")

File /global/cfs/cdirs/desicollab/users/font/Codes/cup1d/cup1d/likelihood/likelihood.py:139, in Likelihood.set_free_parameters(self, free_param_names, free_param_limits)
    136 Nfree = len(self.free_params)
    137 Nin = len(free_param_names)
--> 139 assert Nfree == Nin, "could not setup free parameters"
    141 if self.verbose:
    142     print("likelihood setup with {} free parameters".format(Nfree))

AssertionError: could not setup free parameters
andreufont commented 1 month ago

@jchavesmontero - this is related to the fact that we do not pass something like X_model_fid to the extra theory in the likelihood.py module, like we do for the other nuisances (we pass F_model_fid=self.theory.F_model_fid)

I guess following the other PR (#66 ) we also need a similar different duplication of models for the metals?

This is becoming relevant because I would like to add yet another model for HCD contamination, so we should probably sit down and discuss exactly how these models should be dealt with systematically.

andreufont commented 1 month ago

In the branch fit_desi_edr I solve this issue passing self.metal_models to the extra theory, instead of passing include_metals with labels. As I mentioned above, however, whenever we are both back to work we should discuss this.

andreufont commented 1 month ago

Maybe we don't need to setup an alternative theory object for the high-resolution spectra. This was a hack that probably was a bad idea. It was motivated by the fact that the theory needed to know the redshifts from the data for efficiency, and high-res data could have different zs than low-res data. However, we could easily solve this by add a function called theory.update_zs or similar that merges the redshifts needed by both datasets.

jchavesmontero commented 2 weeks ago

I removed the theory object for high-res data in my last commit. I had to specify zs here and there so it works.