gh-380: cluster hmf bias

[zsirap]

🚀 Pull Request Checklist

✅ Summary

closes #380

🔄 Changes

• implements cluster abundance halo mass function and bias in the observable module
• sets the structure and implements fucntions that will be used for futher galaxy clusters observables

🛠 How to Test

to do : maybe transfer parts of the clusters DEMO notebook

📝 Documentation

to do : check if documentation changes are needed

• This PR updates documentation
• This PR does not require documentation changes
• Issue created for documentation update: Don't forget to link the task!

📌 Additional Notes

tagging remaining galaxy clusters developer leads @m-aguena and @GiorgioLesci

✅ PR Checklist for Developers

• I have titled this PR before merging as "gh-#:", where "#" represents the task it closes
• I have run locally pre-commit using pre-commit run --all-files
• I have tested my changes locally
• No new warnings or errors introduced
• I have updated documentation (if applicable)
• My changes do not introduce breaking changes (i.e: the package still gets installed)
• I have added unit tests (if applicable)
• I have consistently updated the GitHub information for the project, including milestones, task types, and other relevant details.

✅ PR Checklist for Reviewers

• The next PR targets the correct branch
• CI tests have run and passed for the latest commit on the source branch
• Check that the code can still be installed if new packages are imported
• If necessary, the notebooks in the playground will be updated in a corresponding follow-up PR
• Coverage percentage is retained or increased
• Quality of new/changed code is acceptable
• Quality of new/changed unit tests is acceptable
• No data files have been included in the commits
• Implementation follows the agreed task description point by point
• Check that any added folder/file has been added to the README.md file
• Check that the documentation has been updated accordantly
• Check that the corresponding branch has been deleted after merging. If not, delete it

[gcanasherrera]

Dear @zsirap and other clusters contributors,

I'm personally surprised about the length of this PR. This is precisely what we wanted to avoid by interacting closer with you: having more manageable PRs to review. This PR contains modifications to nine (!) files and contains hundreds of lines.

This is not in compliance with our contributors guidelines. Please, could you explain why this PR is so big and what's the reason behind pursuing this approach now?

[GiorgioLesci]

Hello @gcanasherrera

This PR contains modifications to nine (!) files and contains hundreds of lines.

6 files effectively. Two are __init__.py files, and one is a unit test script.

This is not in compliance with our contributors guidelines. Please, could you explain why this PR is so big and what's the reason behind pursuing this approach now?

I'm sorry. In my view, this is very easily manageable. We could keep only the following files:

• matter_statistics.py
• auxiliary.py
• __init__.py

The reason why we included the HMF protocol (in the halo_abundance folder) is to provide context to the content of matter_statistics.py and auxiliary.py. If context (i.e. understanding the structure of the clusters module) is not important, we could keep just the three files I mentioned above

[zsirap]

still in draft but was going to make it not and ask for Amandine and your review after I check a small commit by Aguena done recently ...

tomorrow we have a meeting and maybe after concerting I reply better but as a quick reply we followed the procedure so we opened a first general issue #364 in which we detailed the different smaller issues/PR we are going to open one after another #380 #381 #382 #383 etc ...

it might seems big because it lays the ground structure for the others but logically that's one coherent PR ... so only one observable among three would be implemented ... later there will be e.g. summary statistics PR for each of the 3 cluster probes as well etc ...

if you want to break it more, we can try no worries ....

I see @GiorgioLesci already replied in a more practical way when I was writing this msg so I will let him continue the discussion ...

[gcanasherrera]

Hi @zsirap @GiorgioLesci,

Thanks for your replies! I agree that the PR is scientifically coherent—apologies if my earlier comments focused too much on its size and complexity.

My main concern relates to the guidelines around discussing changes in advance. For example, when I open the issue (#380), it’s difficult to understand what parts of the fork are intended to be merged and what the overall plan is.

The purpose of these guidelines is to avoid unnecessary work and ensure alignment early on. From a first look, a few points stand out that would have benefited from prior discussion:

• The introduction of a new auxiliary.py module within clusters, while we already have a general auxiliary module in cloelib.
• The presence of what seem to be auxiliary-style classes (HaloAbundanceCore, MatterStatistics) that are not located in the auxiliary module.
• The need for a clusters folder itself is not clear.
• It’s also unclear why the protocol is exposed to the user, or why HaloAbundance is defined but not used later in the code.

These are exactly the kinds of design decisions it would be helpful to align on before opening a PR.

[zsirap]

For example, when I open the issue (#380), it’s difficult to understand what parts of the fork are intended to be merged and what the overall plan is.

that I agree ... it was written by @m-aguena when he opened several issues at the same time ... but I should have elaborated more later because the fork should be considered as our clusters group internal code and not an alternative to cloe .. so the user or reader should not check it but rather stay within main cloe and everthing should be (or end up to be) self explanatory ...

we will get back later to the other more technical or design point concerns mentioned which are totally legit at first examination ... no worries

[gcanasherrera]

Sure mates, I'm sure we will make it work, I was just concerned you will end up hating me requesting changes. Remember that we envision to make a full code consistent internally among all probes. The challenge is huge, but I'm sure we can make it work! Ping me back when this is no longer a draft.

[m-aguena]

Sure mates, I'm sure we will make it work, I was just concerned you will end up hating me requesting changes. Remember that we envision to make a full code consistent internally among all probes. The challenge is huge, but I'm sure we can make it work! Ping me back when this is no longer a draft.

@gcanasherrera We are prepared for receiving a bunch of requests for changes, I am used to a multi person PR process and won't take it personally 😉 . As @zsirap mentioned, this PR is still in draft mode as it is a work in progress. But it was good to already have some feedback on the implementations we have, especially with regards of all the auxiliary functions and classes we have.

So, where is the correct place to have those design discussions? Direcly on the issue #380? Or is there a telecon where this type of things are discuessed?

Cheers,

[gcanasherrera]

I guess that at this stage that the PR is still in place, we could continue the discussion here. For the future, please, make the issue a bit more consistent and solid in terms of proposals. Cheers!

[m-aguena]

@gcanasherrera thanks for the pacience, we are leaning how to work with you guys' framework. After your comments, I have a couple of questions:

• Should we move all "auxiliary" functions/classes to the auxiliary module?
• Created a cluster folder in observables because we ended up with ~20 files for clusters in our final code. If you guys are ok with clusters dominating the number of files in observables with ~10 cluster specific files in there (maybe half of our files there can actually be moved to auxiliary), we can remove the clusters folder.

[zsirap]

Should we move all "auxiliary" functions/classes to the auxiliary module?

Created a cluster folder in observables because we ended up with ~20 files for clusters in our final code. If you guys are ok with clusters dominating the number of files in observables with ~10 cluster specific files in there (maybe half of our files there can actually be moved to auxiliary), we can remove the clusters folder.

let's discuss tomorrow among us such major restructuring before we continue here .. in particular we might elaborate on the rationale in the issue on why we considered this structure (if we decide to keep it as is) and continue from there the discussion here with main Cloe maintainers ...

[m-aguena]

@zsirap if we get the @cloe-org/cloe-maintainers feedback, we don't have to keep guessing what is best structure on our side 😉

[gcanasherrera]

please, following what I commented in task #382, create a clusters_helpers.py within the original cloelib auxiliary folder :)

[zsirap]

Hi @gcanasherrera , I thought we might discuss the following in the related issue and not here and that after updating and elaborating the description there, but then i thought maybe it is better to sort things out here and then we update the info there ..

So after discussing among us here is what we reached:

first some clarifications

• In this PR we are just adding the halo abundance observable, but the full cluster implementation also contains two additional observables, all of which are composed of a xx_core file + a protocol file + derived files from protocol. So there will be many additional cluster specific files

• it is true that our auxiliary.py or matter_statistics.py (inside the big cluster folder) or halo_abundance_core.py (inside the halo_abundance sub folder), all look alike a file that should belong to the general cloelib auxiliary folder .. but that applies strongly to our auxiliary.py in which the functions are general ... then much less to matter_statistics.py that has functions more proper or useful to the 3 cluster probes at the same time (that's why it is outside any of the sub folders because it will serve the 3) ... then much more less to halo_abundance_core.py which is closely related to the files inside the halo_abundance sub folder in which it resides ... the reason we created it instead of making its functions as part of the halo_abundance.py protocol was that the functions inside of it are not going to be differentiated later to e.g. Tinker halo mass function or Castro halo mass function while all what is inside halo_abundance.py protocol are meant to ...

• that being said there might be many ways to keep or change the structure going from

a) keep it as it is now as we committed it

to

b) the other side ... which would be moving all what you consider auxiliary alike files to cloelib auxiliary folder + not having any sub folder inside the folder observable + putting all remaining files next or on the same level as the main probes files in the folder observables ...

• but then something in between would be to

c) first drop the big folder cluster
+
move our auxiliary and matter_statistics (though would have liked to keep it in observable folder) to cloelib auxiliary folder
+
keep the clusters sub folders (for time being only one sub folder but as i said 2 more to come) along with either leaving halo_abundance_core.py inside the sub folder or outside of it in the folder observable

We are happy to do any of those even b) though we think things will become crowded and less clear to the user in that option

Two last minor things we can skip replying/discussing if there is nothing more to add about them and focus on the above:

1. "why HaloAbundance is defined but not used later in the code" ... the halo_abundance.py protocol is going to be used/needed and imported in the summary_statitics cluster files later
2. we didn't quite understand what do you mean by "It's also unclear why the protocol is exposed to the user"

[GiorgioLesci]

I would add a more practical view of the option c) described by Ziad. In that case, we would implement

• the cloelib/observables/halo_abundance/ folder, containing
  • halo_abundance.py
  • castro_halo_abundance.py
  • tinker_halo_abundance.py
• in the cloelib/auxiliary/ folder, we'd put the following files we committed to this PR:
  • halo_abundance_core.py, changing its name (e.g. to halo_abundance_helpers.py)
  • matter_statistics.py
  • auxiliary.py, which contains the following methods
    • convert_to_Delta_crit() -> could me moved to ?
    • convert_distance() -> could be moved to cloelib/auxiliary/math_utils.py
    • tabulated_return() -> could be moved to ?
    • the following could be moved to cloelib/cosmology/derived_cosmology.py:
      • photoz_rsd_correction()
      • tophat_window()
      • tophat_window_derivative()
      • isotropic_volume_distance()

The final implementation in observables/ (attained in a few PRs) will consist of the following folders:

• observables/halo_abundance/ (implemented here)
• observables/halo_profile/ -> implementing dark matter density profiles such as NFW
• observables/halo_clustering/

The need for folders is easily explained: we can have potentially ~10 models for the halo mass function and ~5 models for halo profiles. Each model lives in a separate py file.

@zsirap @m-aguena @gcanasherrera please let me know your thoughts

[GiorgioLesci]

Hello @gcanasherrera. One last point we'd like to raise, following Michel's latest commits, is to replace the word "cluster" with "halo" everywhere except in the summary_statistics folder. I detail the reason for this below.

Within the observables/clusters/ folder, we will have the following:

• halo_abundance/ (implemented here)
• halo_profile/ (mass density profiles, e.g. NFW)
• halo_clustering/

These subpackages contain the theory for dark matter halo statistics, which is used not only in galaxy cluster studies, but also in analyses based on galaxy samples (e.g. shape of galaxy mass profiles from galaxy-galaxy lensing, HMF constraints from galaxy abundance).

This renaming would involve the following changes:

• observables/clusters/ -> observables/halos/
• auxiliary/cluster_helpers.py -> auxiliary/halo_helpers.py
• tests/test_cluster_halo_abundance.py -> tests/test_halo_abundance.py