igmhub / picca

set of tools for continuum fitting, correlation function calculation, cosmological fits...
GNU General Public License v3.0
30 stars 22 forks source link

[bump minor] Pk1d covariance new #1067

Closed corentinravoux closed 1 day ago

corentinravoux commented 1 week ago

Two improvements:

corentinravoux commented 1 week ago

image Comparison of covariance matrix calculation on a small example (left before, right after), there are not identical since the definition has changed, but their shape are similar.

Waelthus commented 1 week ago

Ok, this sounds like great improvement, I guess 10 NERSC mins should be much better than the previous version and at least similar structures are obtained. How small is small? E.g. is a smoothing step necessary to make any sense out of the right/bottom 1/3 of the covar? Also do we have an idea where the subtle differences come from? Is this just numerical from e.g. different order of operations or is the difference in method actually analytically expected to produce slightly different results? E.g. at the strong correlation spikes around pixel [0,8] (is that correlation due to the SiII/III correlations? Or something to do with DLAs/ cont fitting? Looks like it's the longest range correlations we have in here, but somewhat weaker in the new approach...) I guess we cannot do inter-redshift-bin correlations just yet, but that should be ok...

Could you run this on a significantly large set of data? I'll go through the code changes now...

corentinravoux commented 1 week ago

This is a very small example, like 2-3% of Y1, just for the test. The main difference between the estimators is that now we are treated the weights properly in the normalization of all the covariance matrix, so it can the profile according the average shape of weights over redshift range. I think it is hard to interpret. For "the strong correlation spikes around pixel [0,8]" I think this is due to the lack of modes that increases the stat error bar at very large scales. This can also be impacted by what you mention.

In Y1 run, associated to systematics (I will show it at the upcoming DESI meeting): image

Waelthus commented 1 week ago

In Y1 run, associated to systematics (I will show it at the upcoming DESI meeting)

Ouch, this looks much more correlated than I expected... Can you chainge the colormap so that it actually goes from -1 to 1 and maybe also that it's white/yellow for 0 correlation and diverging outside? I looked at it and at first glance thought green is zero, but that isn't actually right and it looks like except when comparing the largest and smallest modes available things are >50% correlated everywhere

corentinravoux commented 1 week ago

Be carefull on interpreting that, because the systematic covariance matrix is computed in a very simplified way, thus boosting the correlations. Bootstrap stat only covariance: image stat + syst: image As you can see with the simple syst cov model, it is exagerated

Waelthus commented 1 week ago

maybe we should also add a line for covar computation here: https://github.com/igmhub/picca/blob/02f6c2e61a204254e59e43bbcaecd596d2f8a418/py/picca/tests/test_2_pk1d.py#L88-L97 and compare the output to a saved state. That would at least make sure this code is run regularly (e.g. useful for library version changes) and we don't change stuff by accident, even if the covar of so few spectra will be garbage...

corentinravoux commented 1 day ago

Comments and tests added, waiting for pytest, if successful, let's merge it

Waelthus commented 1 day ago

if one would like to use the bootstrap in testing, maybe allow setting a seed in the command line call via an additional argument and initialize an np.default_rng with that seed if given or without if not, then also the bootstrap could run deterministically. But not fully needed here, and glad there is a test at all

corentinravoux commented 1 day ago

Here since the bootstrap is just a run of the covariance matrix, it is not essential to test it. But yes the seed for the bootstrap can be added later. I think we can merge now