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

nGC covariance #114

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

nGC covariance #114

wants to merge 27 commits into from

Conversation

carlosggarcia
Copy link
Contributor

Implementation of the connected non-Gaussian covariance. At the moment, the trispectrum is computed for matter with a NFW halo profile and multiplied by the galaxy biases if clustering needed. This is probably wrong for the 1h term. More work needed here. At the moment, a warning is raised when using galaxy clustering.

@paulrogozenski
Copy link
Collaborator

The tests are failing as they require a recently-updated version of CCL (LSSTDESC/CCL#1216). This PR implemented a quicker version of calculating trispectrum terms, utilized here in the calculation of the covariance.

@carlosggarcia
Copy link
Contributor Author

@damonge Could you bump the CCL version so we have the latest changes available?

Copy link
Contributor Author

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me. I think we can merge first the SSC and then homogenize the treatment of the halo model stuff so we don't define several of them. We might also want to add a flag to use the HOD for all the trispectra terms, not only the 1h

tests/data/conf_covariance_cNG.yaml Outdated Show resolved Hide resolved

self.cNG_conf = self.config.get("cNG", {})

self.HOD_dict = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to homogenize this with all the other halo model things in the SSC. We can do it in another pull request

Copy link
Contributor Author

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last few comments!

@@ -15,6 +15,8 @@ def covariance_from_name(name):
from .covariance_fourier_gaussian_nmt import FourierGaussianNmt as Cov
elif name == "FourierSSCHaloModel":
from .covariance_fourier_ssc import FourierSSCHaloModel as Cov
elif name == "FouriercNGHaloModel":
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add also the fsky case

cosmo: 'set'

# Setting mask OR fsky approximation
mask_file:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

masks should not be required when using fsky. Remove them and check that nothing breaks (I think we already tested this for the SSC)

a_pivot: 1.0
ns_independent: False

GaussianFsky:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since now more components apart from the Gaussian part will be using fsky, could you move this to the tjpcov section? i.e.

tjpcov:
    fsky: 0.05

(and update the other parts of the code correspondingly)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only difference from the other test file is the way you compute the fsky. Cannot we merge both files so we don't need to maintain two files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it is too cumbersome, i'm fine to leave it as is for now

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could and parameterize over where to find fsky along with the bin combinations. There would be a bit of work to clean up each test combination (for gaussian, cng, and ssc) and we would need to make two sets of functions to load in the two different yamls and such. I think this format if fine for the scope of the PR and we can clean this up in a subsequent one. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking to merge it only for the cNG but I agree we can leave it as is for now.

Copy link
Contributor Author

@carlosggarcia carlosggarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let's wait until Monday to see if the new version of pyccl appears in conda-forge and merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking to merge it only for the cNG but I agree we can leave it as is for now.

@carlosggarcia carlosggarcia marked this pull request as ready for review January 24, 2025 19:10
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.

2 participants