Skip to content

gh-85: add correlation function class within summary statistics#92

Merged
marcobonici merged 18 commits into
mainfrom
85_add_correlation_function_class_within_summary_statistics
May 8, 2025
Merged

gh-85: add correlation function class within summary statistics#92
marcobonici merged 18 commits into
mainfrom
85_add_correlation_function_class_within_summary_statistics

Conversation

@llinke1

@llinke1 llinke1 commented Mar 12, 2025

Copy link
Copy Markdown
Collaborator

Added the class CorrelationFunction to the "summary_statistics" folder as requested in #85
This class can compute projected correlation functions (i.e. the cosmic shear functions $\xi_+$ and $\xi_-$ and their counterparts for galaxy clustering and galaxy-galaxy-lensing), using an already defined AngularTwoPointobject.

The Hankel transforms are carried out by explicit summation over the Wigner-$d$-matrices:
$$ \xi^{ab}\pm(\theta) = \sum\ell\frac{2\ell+1}{4\pi},(\pm1)^{s_b}, C^{ab\pm}\ell,d^\ell{s_a,\pm s_b}(\theta) $$
This is the accurate representation without a flat-sky approximation, but a fast Hankel transform with FFTLog might be a lot faster.

I also added the notebook compare_c_ell_ccl.ipynb, which tests the $C(\ell)$ and $\xi$ against ccl.
A high number of $\ell$ modes is required for accurate Hankel transforms (30000 - 60000), but with this high number the $\xi$ are of similar accuracy as the $C(\ell)$ at scales between 5' and 100'. $\xi_-$shows unsurprisingly the worst agreement, as it depends the strongest on the high-$\ell$ modes.

@llinke1 llinke1 linked an issue Mar 12, 2025 that may be closed by this pull request
@gcanasherrera gcanasherrera changed the title 85 add correlation function class within summary statistics gh-85: add correlation function class within summary statistics Mar 12, 2025
@gcanasherrera gcanasherrera added enhancement New feature or request weak-lensing Tasks related to the photometric observables labels Mar 12, 2025
@gcanasherrera gcanasherrera added this to the v1.0.0 milestone Mar 12, 2025
@marcobonici

Copy link
Copy Markdown
Collaborator

Hi @llinke1 , thx for the PR! I'll start reviewing it:)
Before I start: which is your opinion on the FFTLog? Should we already implement one within this PR or postpone it to a new issue? Eventually, we might go with hankl or mcfit, the latter being already jax compatible.
I would add it into another issue, but let me know what you think!

@llinke1

llinke1 commented Mar 13, 2025

Copy link
Copy Markdown
Collaborator Author

Thanks @marcobonici!

I think I would prefer the FFTLog implementation to be a separate issue. To me it is useful to already have a working (although slow) implementation of correlation functions, so the later FFTLog implementation could be compared against it.

I am happy to help with the FFTLog implementation of course, although I am not yet familiar with mcfit and not confident enough in my jaxknowledge to port hankl.

@marcobonici

Copy link
Copy Markdown
Collaborator

Hi @llinke1 , first and foremost sorry for keeping this PR open for a while.
I am generally happy with this PR. Before continuing the review process, a few comments:

  • Update the PR branch. After opening this PR, the main branch changed significantly. Please update your branch accordingly.
  • Notebook and comparison. While the main branch was updated, several things improved regarding the validation (see @josecolomanadal 's notebook in the playground repo). I would like to see the validation done again, since it's likely the results will be better. Also, the notebooks need to be moved to the playground (but I'll let @gcanasherrera if she has any further comment on this point).
  • Protocols. After opening this PR, we changed the structure of the code, using protocols. I think it would be valuable to use this programming paradigm even with the correlation functions, in order to ensure homogeneity. Let us know if you need help/references with that.
  • Jax and FFTLog. I think the jax compatibility is very good, although not yet complete (but this is something I can take care by myself in the future). I also agree that the super performant FFTLog based transformation might be an overkill for the moment, and we can postpone it (I will open a task on this, which will use your actual implementation as a safe benchmark to check accuracy).

Let me know if you need to discuss anything.
Marco

@llinke1

llinke1 commented Apr 17, 2025

Copy link
Copy Markdown
Collaborator Author

Hi @marcobonici ,
Thank you for the review! I will update the branch to be compatible with the changes in the main branch and implement protocols. I have not yet used them, so might come back to you with questions. I'll move the example notebook to the playgroundbut I am not sure if I have contribution rights there?

@marcobonici

Copy link
Copy Markdown
Collaborator

Very quickly:

  • regarding protocols, reach us whenever you have questions!
  • regarding the playground, you should have reading access, so you should be able to fork and open a PR

@gcanasherrera

Copy link
Copy Markdown
Member

@llinke1 @marcobonici Laila already have written permissions here, it does not make sense to go with forks :) Just change @llinke1 role in playground

@llinke1

llinke1 commented Apr 24, 2025

Copy link
Copy Markdown
Collaborator Author

Hi @marcobonici ,
I have made the requested changes:

  • All changes to the main branch are incorporated
  • I have added an angular_correlation_functionprotocol and renamed my class to angular_correlation_function_wigner. This will make it easier to add angular_correlation_function_fftlog later. I hope this fits what you had in mind.
  • I have removed the notebook, but reran the validation test. As you anticipated, the agreement with ccl is much better now.
    image

Should I create a PR inplaygroundwith this notebook now, or should I wait for approval of this PR?

@marcobonici

Copy link
Copy Markdown
Collaborator

Hi @llinke1 sorry for the delay, but I was sick.
After we merge this, please open a PR in the playground repository with the notebook you created.
I will just ask the JC1 leads a cross-check, to ensure there are no issues on the scientific side, but we should be good to go :)

CCing @martincrocce @CarmelitaCarbone @itutusaus

@marcobonici marcobonici merged commit 35484b5 into main May 8, 2025
@marcobonici marcobonici deleted the 85_add_correlation_function_class_within_summary_statistics branch May 8, 2025 04:40
@gcanasherrera

Copy link
Copy Markdown
Member

@all-contributors please add @llinke1 for code

@allcontributors

Copy link
Copy Markdown
Contributor

@gcanasherrera

I've put up a pull request to add @llinke1! 🎉

santiagocasas pushed a commit that referenced this pull request Jul 7, 2025
…s_within_summary_statistics

gh-85: add correlation function class within summary statistics
didamarkovic pushed a commit that referenced this pull request Jul 10, 2025
…s_within_summary_statistics

gh-85: add correlation function class within summary statistics
chiaramoretti pushed a commit that referenced this pull request Aug 1, 2025
…s_within_summary_statistics

gh-85: add correlation function class within summary statistics
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request weak-lensing Tasks related to the photometric observables

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add correlation_function class within summary statistics

3 participants