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

Fix ell range #60

Merged
merged 6 commits into from
Jul 10, 2023
Merged

Fix ell range #60

merged 6 commits into from
Jul 10, 2023

Conversation

sgiardie
Copy link
Collaborator

@sgiardie sgiardie commented Jul 4, 2023

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.

@sgiardie sgiardie requested a review from xgarrido July 4, 2023 13:30
@xgarrido
Copy link
Collaborator

xgarrido commented Jul 4, 2023

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
Copy link
Collaborator Author

sgiardie commented Jul 4, 2023

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
Copy link
Collaborator

xgarrido commented Jul 4, 2023

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

_release = "v0.7.1"

@xgarrido
Copy link
Collaborator

xgarrido commented Jul 5, 2023

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

@sgiardie
Copy link
Collaborator Author

sgiardie commented Jul 5, 2023

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
Copy link

codecov-commenter commented Jul 5, 2023

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

@@            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 Coverage Δ
mflike/theoryforge.py 85.32% <ø> (+0.54%) ⬆️
mflike/mflike.py 89.50% <100.00%> (ø)

@sgiardie
Copy link
Collaborator Author

sgiardie commented Jul 5, 2023

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
Copy link
Collaborator Author

sgiardie commented Jul 5, 2023

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
Copy link
Member

HTJense commented Jul 5, 2023

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
Copy link
Collaborator

xgarrido commented Jul 5, 2023

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
Copy link
Collaborator

xgarrido commented Jul 5, 2023

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
Copy link
Collaborator Author

sgiardie commented Jul 6, 2023

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
Copy link
Collaborator Author

sgiardie commented Jul 6, 2023

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
Copy link
Collaborator

xgarrido commented Jul 6, 2023

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
Copy link
Collaborator

xgarrido commented Jul 6, 2023

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
Copy link
Collaborator Author

sgiardie commented Jul 7, 2023

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
Copy link
Collaborator

mgerbino commented Jul 7, 2023

@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
Copy link
Collaborator Author

sgiardie commented Jul 7, 2023

@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
Copy link
Member

HTJense commented Jul 7, 2023

@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
Copy link
Collaborator

mgerbino commented Jul 7, 2023

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
Copy link
Collaborator

mgerbino commented Jul 7, 2023

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
Copy link
Collaborator Author

sgiardie commented Jul 7, 2023

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
Copy link
Collaborator

mgerbino commented Jul 7, 2023

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

@xgarrido xgarrido merged commit 6527b41 into master Jul 10, 2023
12 checks passed
@xgarrido xgarrido deleted the fix_scales branch July 10, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants