SWIFTSIM / HBTplus

HBTplus halo finder adapted for the FLAMINGO and COLIBRE simulations
0 stars 0 forks source link

Prefer to use particles in TracerParticleTypes as tracers for unresolved subhalos #12

Closed jchelly closed 2 months ago

jchelly commented 4 months ago

This PR will modify the code to prefer to use particles of the types specified in TracerParticleTypes as tracers for unresolved ("orphan") subhalos. This should minimize the chance of having orphan subhalos where the tracer particle ceases to exist and we don't have a position for the subhalo.

To do:

jchelly commented 4 months ago

If I set MinNumTracerPartOfSub=1 then in the small COLIBRE test all of the resolved subhalos contain tracer particles, as expected. But there is a small number of orphans (2 of ~40,000) assigned to non-tracer particles. I'm not sure where those are coming from.

robjmcgibbon commented 4 months ago

Hi John, is this ready for review, or are you looking into the orphans assigned to non-tracer particles?

jchelly commented 4 months ago

I'm still looking into the orphans on non-tracers. I've also found that if I update this from master it crashes on a subhalo with zero particles which has never been assigned a most bound particle ID.

jchelly commented 4 months ago

Number of tracers of each type on FLAMINGO L1000N0900/HYDRO_FIDUCIAL with TracerParticleTypes 1 4:

Type 0 : found 1077 most bound particles
Type 1 : found 3484740 most bound particles
Type 2 : found 0 most bound particles
Type 3 : found 0 most bound particles
Type 4 : found 937871 most bound particles
Type 5 : found 2 most bound particles

So mostly correct but there are a few orphans placed on gas and BH particles here too.

jchelly commented 4 months ago

The orphans on the wrong type particles can happen in two ways. One way is much more likely:

VictorForouhar commented 3 months ago

What else needs to be done before merging?

jchelly commented 3 months ago

Pull requests #20 and #23 are based on this branch, so I guess we need to decide what order we merge things in.

VictorForouhar commented 3 months ago

I will include the changes made by #20 in #23, since I want to have orphans with collisionless tracers only. Hence, #20 should be merged first .

VictorForouhar commented 3 months ago

Note that I have a set of COLIBRE outputs based on the different forks associated to this branch, to check if something has gone wrong at a given point. Masking fix are run with #20, Orphan tracers were run with this branch commit ef54c1f24474c13942ab69559ac001eac21acc64, Multiple tracers on the current version of this branch, and Zero orphans with #23. image

VictorForouhar commented 2 months ago

Should we begin merging this branch into master? I can work on that today, and test whether the merged version yields the same objects as this branch, since your other changes are in principle orthogonal to these.

jchelly commented 2 months ago

Yes, I think that makes sense. All of our recent testing has been done on top of this branch anyway.

VictorForouhar commented 2 months ago

I am running the COLIBRE small test box.

VictorForouhar commented 2 months ago

The number of structures is the exact same after merging the branches. The bound mass functions are also the same: image Given that merged changes affected very different parts of the code, I believe this small test shows nothing is expected to change dramatically if we were to test on FLAMINGO explicitly.

VictorForouhar commented 2 months ago

Everything looks okay to me; the remaining comments I have do not need to be addressed in this pull request necessarily.

VictorForouhar commented 2 months ago

My latest commit has been tested by seeing if the number of orphans/subhalos change, as well as the bound mass function. I also added a script to check consistency of the tracer IDs between two catalogues. Here is an example comparison between HBT runs done before and after 24f85a1

Testing orphan MostBoundParticleId consistency across two HBT+ catalogues
Opening HBT catalogue: /cosma7/data/dp004/dc-foro1/colibre/hbt_testing/merged_master/006/SubSnap_006.{file_nr}.hdf5 --- DONE
Opening HBT catalogue: /cosma7/data/dp004/dc-foro1/colibre/hbt_testing/no_tracer_saving/006/SubSnap_006.{file_nr}.hdf5 --- DONE
Reference catalogue has 2904 orphans; other catalogue has 2904.
A total of 0 orphans disagree about their MostBoundId.

Testing orphan MostBoundParticleId consistency across two HBT+ catalogues
Opening HBT catalogue: /cosma7/data/dp004/dc-foro1/colibre/hbt_testing/merged_master/007/SubSnap_007.{file_nr}.hdf5 --- DONE
Opening HBT catalogue: /cosma7/data/dp004/dc-foro1/colibre/hbt_testing/no_tracer_saving/007/SubSnap_007.{file_nr}.hdf5 --- DONE
Reference catalogue has 6400 orphans; other catalogue has 6400.
A total of 0 orphans disagree about their MostBoundId.

Testing orphan MostBoundParticleId consistency across two HBT+ catalogues
Opening HBT catalogue: /cosma7/data/dp004/dc-foro1/colibre/hbt_testing/merged_master/008/SubSnap_008.{file_nr}.hdf5 --- DONE
Opening HBT catalogue: /cosma7/data/dp004/dc-foro1/colibre/hbt_testing/no_tracer_saving/008/SubSnap_008.{file_nr}.hdf5 --- DONE
Reference catalogue has 11177 orphans; other catalogue has 11177.
A total of 0 orphans disagree about their MostBoundId.

Testing orphan MostBoundParticleId consistency across two HBT+ catalogues
Opening HBT catalogue: /cosma7/data/dp004/dc-foro1/colibre/hbt_testing/merged_master/009/SubSnap_009.{file_nr}.hdf5 --- DONE
Opening HBT catalogue: /cosma7/data/dp004/dc-foro1/colibre/hbt_testing/no_tracer_saving/009/SubSnap_009.{file_nr}.hdf5 --- DONE
Reference catalogue has 16607 orphans; other catalogue has 16607.
A total of 0 orphans disagree about their MostBoundId.

Testing orphan MostBoundParticleId consistency across two HBT+ catalogues
Opening HBT catalogue: /cosma7/data/dp004/dc-foro1/colibre/hbt_testing/merged_master/010/SubSnap_010.{file_nr}.hdf5 --- DONE
Opening HBT catalogue: /cosma7/data/dp004/dc-foro1/colibre/hbt_testing/no_tracer_saving/010/SubSnap_010.{file_nr}.hdf5 --- DONE
Reference catalogue has 22032 orphans; other catalogue has 22032.
A total of 0 orphans disagree about their MostBoundId.
VictorForouhar commented 2 months ago

Latest change has also been checked against previous version before committing. I do not envision other changes being made on my side, unless @robjmcgibbon spots something I have not.