search

simonsobs / LAT_MFLike

Multifrequency Likelihood for SO Large Aperture Telescope
https://lat-mflike.readthedocs.io
Other
4 stars 12 forks source link

Fix ell range #60

Closed sgiardie closed 1 year ago

sgiardie commented 1 year ago

Fixing the ell range with a less conservative one, i.e. from [50,5000] to [30,9000], which should be used when marginalizing over foregrounds.

Plus, I just fixed some comments regarding bandpass construction.

xgarrido commented 1 year ago

I was waiting the end of the unit test to comment but it's failing since the lmax value is too high wrt to the simulated dataset. So we can decrease the lmin value without problem but the lmax value must be related to the lmax in the simulation dataset. To merge this PR we need an associated new simulation dataset.

sgiardie commented 1 year ago

I was waiting the end of the unit test to comment but it's failing since the lmax value is too high wrt to the simulated dataset. So we can decrease the lmin value without problem but the lmax value must be related to the lmax in the simulation dataset. To merge this PR we need an associated new simulation dataset.

Right, I haven't thought about the dataset. Should we use the set of ideal simulations I generated up to ell 9000? I could add them to the nersc path where the current sims are stored (under a v0.8.5 folder?)

xgarrido commented 1 year ago

If you can upload your simulation data set (which is also related to the systematics paper) it will be great.

Regarding the version, the dataset version (currently 0.7.1) is not particularly synchronized with the mflike software release (currently 0.8.5). We try to do that at the beginning but software development has diverged without the need to regenerate the dataset. I think your simulation dataset should be 0.8.0. You can upload the tarball in NERSC (I mean you should have the rights to write within the /global/cfs/cdirs/sobs/www/users/MFLike_data directory (I see you already tried). Finally you have to upgrade the version number https://github.com/simonsobs/LAT_MFLike/blob/4fdfb2e08fa747d8f9baf7eda542e57e6dd8876a/mflike/mflike.py#L23

xgarrido commented 1 year ago

Another issue related to the change of dataset is the unit tests. You will need to update all the chi2 values...

sgiardie commented 1 year ago

Another issue related to the change of dataset is the unit tests. You will need to update all the chi2 values...

Sure, on it. I will also update the notebook (since I have used slightly different fiducial params to generate those sims)

codecov-commenter commented 1 year ago

Codecov Report

Merging #60 (8117a46) into master (4fdfb2e) will increase coverage by 0.27%. The diff coverage is 100.00%.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/60/graphs/tree.svg?width=650&height=150&src=pr&token=qrrVcbNCs5&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs)](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/60?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs) ```diff @@ Coverage Diff @@ ## master #60 +/- ## ========================================== + Coverage 87.26% 87.53% +0.27% ========================================== Files 3 3 Lines 369 369 ========================================== + Hits 322 323 +1 + Misses 47 46 -1 ``` | [Impacted Files](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/60?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs) | Coverage Δ | | |---|---|---| | [mflike/theoryforge.py](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/60?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs#diff-bWZsaWtlL3RoZW9yeWZvcmdlLnB5) | `85.32% <ø> (+0.54%)` | :arrow_up: | | [mflike/mflike.py](https://app.codecov.io/gh/simonsobs/LAT_MFLike/pull/60?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=simonsobs#diff-bWZsaWtlL21mbGlrZS5weQ==) | `89.50% <100.00%> (ø)` | |
sgiardie commented 1 year ago

Ok I have added the v0.8 sims on NERSC, updated the test and the notebook. The test now takes a bit of time to run, since I added also high accuracy camb parameters (that I used also to generate the sims).

sgiardie commented 1 year ago

Could it make sense to make a version of the example yaml file where the theory block uses cosmopower instead of camb? This is useful for having faster runs when we need high accuracy. Would it mean that cosmopower should become an mflike dependency though? @HTJense

HTJense commented 1 year ago

Could do that, but in the current state, this would add 3 external dependencies (cosmopower, soliket (for the cobaya wrapper), and the actual pre-trained networks), which might be a bit overkill for some of the things you want to do for now. Especially the tensorflow requirement for cosmopower is quite heavy on resources for just a single test.

xgarrido commented 1 year ago

We don't want cosmopower as a mflike dependency (I think it's optional for soliket and it's better like that). As @HTJense said is a bit overkill but I am fine for adding a cosmopower yaml example file. It's up to the user to install all the stuff to play with it.

xgarrido commented 1 year ago

Regarding the time the unit tests take, since we are checking only chi2 values, we do not really need high accuracy settings. I understand that you have done the simulation dataset with high accuracy settings and it's great that you have added the default accuracy settings within the yaml example file, but regarding the unit tests, I think we don't care; the tests are just here for ensuring we do not break anything when changing mflike software. The chi2 values inside unit tests must not be taken as reference values of any kind.

sgiardie commented 1 year ago

Regarding the time the unit tests take, since we are checking only chi2 values, we do not really need high accuracy settings. I understand that you have done the simulation dataset with high accuracy settings and it's great that you have added the default accuracy settings within the yaml example file, but regarding the unit tests, I think we don't care; the tests are just here for ensuring we do not break anything when changing mflike software. The chi2 values inside unit tests must not be taken as reference values of any kind.

Ok, so I will switch the test to low accuracy settings

sgiardie commented 1 year ago

I think I may also set the camb params in (at least part of) the notebook to low accuracy, since also the notebook is tested when each commit is pushed and it is taking some time

xgarrido commented 1 year ago

I think I may also set the camb params in (at least part of) the notebook to low accuracy, since also the notebook is tested when each commit is pushed and it is taking some time

Agree it surely changes a bit the chi2 value within the notebook but we are not really quantitative in the notebook

xgarrido commented 1 year ago

I approve the PR but I forget : can you add a new v0.8 section in the README https://github.com/simonsobs/LAT_MFLike#simulation-releases ?

sgiardie commented 1 year ago

I approve the PR but I forget : can you add a new v0.8 section in the README https://github.com/simonsobs/LAT_MFLike#simulation-releases ?

Sure, done it!

mgerbino commented 1 year ago

@sgiardie , I have a question on how the theory (CMB) Cls are calculated. In the current version, they are computed up to lmax_theory, which is quite high for CMB (and certainly slows down the calculation)...shall we introduce an ell_max_CMB<lmax_theory?

sgiardie commented 1 year ago

@sgiardie , I have a question on how the theory (CMB) Cls are calculated. In the current version, they are computed up to lmax_theory, which is quite high for CMB (and certainly slows down the calculation)...shall we introduce an ell_max_CMB<lmax_theory?

In my understanding, CAMB adds some additional ells to the lmax you pass to it. I am not sure how this is done within CAMB, I remember looking to that in the past and not finding a conclusive answer. So I think it would be a bit dangerous to set a different lmax with respect to lmax_theory if we don't know exactly how CAMB computes the lmax to use

HTJense commented 1 year ago

@sgiardie If you request Cells up to lmax, then camb will compute them up to lmax + lens_margin. This is to ensure that your Cells are accurate enough due to lensing effects at high ell (which couple different l modes). By default, this lens_margin parameter is set to 150. So right now MFLike requests up to l = 9000, which means (at default accuracy) camb will compute Cells up to l = 9150.

From camb.model:

def set_for_lmax(self, lmax, max_eta_k=None, lens_potential_accuracy=0,
                 lens_margin=150, k_eta_fac=2.5, lens_k_eta_reference=18000.0, nonlinear=None):
    if self.DoLensing:
        self.max_l = lmax + lens_margin
    else:
        self.max_l = lmax
mgerbino commented 1 year ago

my point is that probably we don't know to compute the CMB up to 9000. CMB is dying already below 6000, where it is already largely dominated by foreground. Depending on the accuracy settings (and provided one is not using emulators), it is likely useless to spend computational time in getting Cls up to 9000+margin

mgerbino commented 1 year ago

The other issue is that, when you run the example notebook, there is a little inconsistency: in block 47 here, the cls got from camb (and thus computed up to lmax_theory+margin) are cut in the range lmin,lmax, where lmax=lmax_theory=9000. However, for the v0.8 dataset, it seems to me that max(l_bpws) is 9002, so slightly larger than lmax_theory. This causes a shape mismatch when you try to combine the dls (cur from the camb cls) and the foreground spectra (which are instead computed in the l_bpws range).

Maybe this is not propagated as an error in mflike, but I would make sure that the lmax_theory is always larger than max(l_bpws) regardless of the extra ell tolerance added by camb.

sgiardie commented 1 year ago

The other issue is that, when you run the example notebook, there is a little inconsistency: in block 47 here, the cls got from camb (and thus computed up to lmax_theory+margin) are cut in the range lmin,lmax, where lmax=lmax_theory=9000. However, for the v0.8 dataset, it seems to me that max(l_bpws) is 9002, so slightly larger than lmax_theory. This causes a shape mismatch when you try to combine the dls (cur from the camb cls) and the foreground spectra (which are instead computed in the l_bpws range).

Maybe this is not propagated as an error in mflike, but I would make sure that the lmax_theory is always larger than max(l_bpws) regardless of the extra ell tolerance added by camb.

In the notebook in this branch (fix_scales) I have used the ell range [2, mflike.l_bpws[-1] +1] to generate the theoretical spectra, in order to arrive up to the lmax of the data.

I would say that the theory spectra in mflike are always generated at least up to the lmax of the data, see here:

 def get_requirements(self):
    return dict(Cl={k: max(c, self.lmax_theory + 1) for k, c in self.lcuts.items()})
mgerbino commented 1 year ago

ok, thanks! I missed the fix in the notebook!