Modifications related to the mass swapping fixes and other minor changes#25
Open
angel-chandro wants to merge 15 commits intoICRAR:develfrom
Open
Modifications related to the mass swapping fixes and other minor changes#25angel-chandro wants to merge 15 commits intoICRAR:develfrom
angel-chandro wants to merge 15 commits intoICRAR:develfrom
Conversation
…es or without them)
… different to z=0, removing unused parts and other bits
…halo_type is being changed but the host halo ID is not. This is leading to type 0 galaxies associated to satellite subhalos and type 1 galaxies to central subhalos when looking at the output variables id_halo_tree and id_subhalo_tree.
Reviewer's GuideThis PR refines how halo and subhalo properties are defined under the mass-swapping fix, propagates those properties consistently to galaxies (especially type 2), corrects spin/virial-velocity usage in several physics modules, tightens transient/edge-case handling, and removes obsolete code and a Python helper script. Sequence diagram for transferring galaxies between snapshots with mass-swapping fixsequenceDiagram
participant SharkRunner_impl as SharkRunner_impl
participant Evolve as transfer_galaxies_to_next_snapshot
participant Subhalo as Subhalo
participant Galaxy as Galaxy
SharkRunner_impl->>Evolve: transfer_galaxies_to_next_snapshot(all_halos_this_snapshot, snapshot, all_baryons, dark_matter_params)
loop for each halo and subhalo
Evolve->>Subhalo: check_subhalo_galaxy_composition()
Evolve->>Evolve: adjust_main_galaxy(parent_subhalo, descendant_subhalo, dark_matter_params)
activate Evolve
alt main_galaxy is type2
alt dark_matter_params.apply_fix_to_mass_swapping_events == true
Evolve->>Galaxy: set type2 props from parent_subhalo.concentration_infall, Mvir_infall, lambda_infall, Vvir_infall
else dark_matter_params.apply_fix_to_mass_swapping_events == false
Evolve->>Galaxy: set type2 props from parent_subhalo.concentration, Mvir, lambda, Vvir
end
end
Evolve-->>Subhalo: transfer_galaxies_to(descendant_subhalo)
deactivate Evolve
end
Evolve-->>SharkRunner_impl: done for snapshot
Updated class diagram for halo, subhalo, and galaxy mass-swapping propertiesclassDiagram
class DarkMatterHaloParameters {
bool apply_fix_to_mass_swapping_events
bool use_converged_lambda_catalog
double min_part_convergence
}
class DarkMatterHalos {
halo_virial_velocity(m, z)
halo_virial_radius(m, z)
nfw_concentration(m, z)
halo_lambda(subhalo, m, z, npart)
}
class Halo {
int id
double Mvir
double Vvir
double concentration
double lambda
int snapshot
Subhalo* central_subhalo
vector~SubhaloPtr~ satellite_subhalos
}
class Subhalo {
int id
int snapshot
int descendant_snapshot
int last_snapshot_identified
bool has_descendant
bool has_mainprogenitor
bool main_progenitor
bool IsInterpolated
bool transient_candidate
subhalo_type_t subhalo_type
double Mvir
double Mvir_infall
double Vvir
double Vvir_infall
double rvir_infall
double concentration
double concentration_infall
double lambda
double lambda_infall
Vector3 L
Vector3 L_infall
Halo* host_halo
}
class Galaxy {
galaxy_type_t galaxy_type
double concentration_type2
double msubhalo_type2
double lambda_type2
double vvir_type2
double r_rps
struct bulge_gas
}
class TreeBuilder {
define_central_subhalos(trees, sim_params, dark_matter_params, darkmatterhalos)
define_central_subhalo(halo, subhalo, sim_params, dark_matter_params, darkmatterhalos, final_snap)
define_ages_halos(trees, sim_params, darkmatterhalos)
spin_interpolated_halos(trees, sim_params)
remove_satellite(halo, subhalo)
}
DarkMatterHaloParameters <-- TreeBuilder : uses
DarkMatterHalos <-- TreeBuilder : uses
Halo "1" o-- "many" Subhalo : all_subhalos
Halo "1" o-- "1" Subhalo : central_subhalo
Subhalo --> Halo : host_halo
Subhalo "1" --> "many" Galaxy : hosts_galaxies
DarkMatterHaloParameters <-- DarkMatterHalos : params
class GalaxyMergers {
merging_timescale(galaxy, primary, secondary, simparams, cosmology, dark_matter_params)
}
DarkMatterHaloParameters <-- GalaxyMergers : uses
class EvolveHalos {
transfer_galaxies_to_next_snapshot(halos, snapshot, AllBaryons, dark_matter_params)
adjust_main_galaxy(parent, descendant, dark_matter_params)
}
DarkMatterHaloParameters <-- EvolveHalos : uses
class GasCooling {
cooling_rate(subhalo, galaxy, z, delta_t, dark_matter_params)
}
DarkMatterHaloParameters <-- GasCooling : uses
class DarkMatterHalos_subhalo_time {
subhalo_dynamical_time(subhalo, z)
}
DarkMatterHaloParameters <-- DarkMatterHalos_subhalo_time : uses
Subhalo <-- DarkMatterHalos_subhalo_time : input
Subhalo <-- DarkMatterHalos : halo_lambda_input
Subhalo <-- TreeBuilder : define_central_subhalo
Halo <-- TreeBuilder : define_central_subhalo
Subhalo <-- TreeBuilder : define_ages_halos
Galaxy <-- GalaxyMergers : merging_timescale_updates
Galaxy <-- EvolveHalos : adjust_main_galaxy_updates
Subhalo <-- GasCooling : cooling_rate_uses
Galaxy <-- GasCooling : cooling_rate_uses
Flow diagram for GasCooling::cooling_rate virial velocity and hot gas fraction selectionflowchart TD
A[Start cooling_rate] --> B[Compute base vvir = subhalo.Vvir]
B --> C[Compute base fhot = mhot / subhalo.Mvir]
C --> D{subhalo.subhalo_type == CENTRAL && apply_fix_to_mass_swapping_events?}
D -- Yes --> E[Set vvir = subhalo.host_halo.Vvir]
E --> F[Set fhot = mhot / subhalo.host_halo.Mvir]
D -- No --> G[Keep base vvir and fhot]
E --> H
G --> H{subhalo.subhalo_type == SATELLITE && subhalo.Vvir_infall != 0 && apply_fix_to_mass_swapping_events?}
H -- Yes --> I[Set vvir = subhalo.Vvir_infall]
I --> J[Set fhot = mhot / subhalo.Mvir_infall]
H -- No --> K[Use current vvir and fhot]
J --> L[Continue with cooling radius and rate calculations]
K --> L[Continue with cooling radius and rate calculations]
L --> M[End cooling_rate]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In DarkMatterHalos::subhalo_dynamical_time, r and v are no longer initialised for the default (no mass-swapping) case, so if neither of the apply_fix_to_mass_swapping_events branches trigger, r and v stay at 0 and will lead to invalid dynamics; consider restoring a default assignment (e.g. using subhalo.Mvir and subhalo.Vvir) before the conditional branches.
- adjust_main_galaxy and transfer_galaxies_to_next_snapshot now take DarkMatterHaloParameters by value, which causes an unnecessary copy each call; consider passing it as a const reference instead unless mutation is required.
- In TreeBuilder::define_central_subhalo, when apply_fix_to_mass_swapping_events is true you use the host halo mass (mvir = halo->Mvir) to recompute lambda but still compute npart from the subhalo mass; if npart is meant to represent the particle count of the mass used for lambda, it might be more consistent to derive it from mvir rather than subhalo->Mvir.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In DarkMatterHalos::subhalo_dynamical_time, r and v are no longer initialised for the default (no mass-swapping) case, so if neither of the apply_fix_to_mass_swapping_events branches trigger, r and v stay at 0 and will lead to invalid dynamics; consider restoring a default assignment (e.g. using subhalo.Mvir and subhalo.Vvir) before the conditional branches.
- adjust_main_galaxy and transfer_galaxies_to_next_snapshot now take DarkMatterHaloParameters by value, which causes an unnecessary copy each call; consider passing it as a const reference instead unless mutation is required.
- In TreeBuilder::define_central_subhalo, when apply_fix_to_mass_swapping_events is true you use the host halo mass (mvir = halo->Mvir) to recompute lambda but still compute npart from the subhalo mass; if npart is meant to represent the particle count of the mass used for lambda, it might be more consistent to derive it from mvir rather than subhalo->Mvir.
## Individual Comments
### Comment 1
<location path="src/dark_matter_halos.cpp" line_range="147-152" />
<code_context>
double r = 0;
double v = 0;
- r = halo_virial_radius(subhalo.Mvir, z);;
- v = subhalo.Vvir;
-
if(subhalo.subhalo_type == Subhalo::CENTRAL &&
params.apply_fix_to_mass_swapping_events){
r = halo_virial_radius(subhalo.host_halo->Mvir, z);
- v = subhalo.Vvir;
+ v = subhalo.host_halo->Vvir;
}
else if(subhalo.subhalo_type == Subhalo::SATELLITE && subhalo.rvir_infall != 0 &&
params.apply_fix_to_mass_swapping_events){
</code_context>
<issue_to_address>
**issue (bug_risk):** r and v can remain unset when mass-swapping fix is disabled, leading to invalid dynamical times
With the generic `r`/`v` initialisation removed, when `params.apply_fix_to_mass_swapping_events` is false neither branch runs and both remain `0`. This changes behaviour from the previous version and can produce `tdyn = 0/0` or `0`, which is likely a bug. Please reintroduce the default initialisation (e.g. `r = halo_virial_radius(subhalo.Mvir, z); v = subhalo.Vvir;`) before the conditional and override it only when the fix is applied.
</issue_to_address>
### Comment 2
<location path="src/evolve_halos.cpp" line_range="36-37" />
<code_context>
namespace shark {
-void adjust_main_galaxy(const SubhaloPtr &parent, const SubhaloPtr &descendant)
+void adjust_main_galaxy(const SubhaloPtr &parent, const SubhaloPtr &descendant,
+ DarkMatterHaloParameters dark_matter_params)
{
// A subhalo that is not main progenitor of its descendant cannot
</code_context>
<issue_to_address>
**suggestion (performance):** Pass DarkMatterHaloParameters by const reference instead of by value
Here and in `transfer_galaxies_to_next_snapshot`, `DarkMatterHaloParameters` is copied on each call. Since it’s configuration and not modified, please pass it as `const DarkMatterHaloParameters&` to avoid unnecessary copies and match the usual calling convention in this codebase.
</issue_to_address>
### Comment 3
<location path="include/evolve_halos.h" line_range="50" />
<code_context>
* @param AllBaryons The TotalBaryon accummulation object
*/
-void transfer_galaxies_to_next_snapshot(const std::vector<HaloPtr> &halos, int snapshot, TotalBaryon &AllBaryons);
+void transfer_galaxies_to_next_snapshot(const std::vector<HaloPtr> &halos, int snapshot, TotalBaryon &AllBaryons, DarkMatterHaloParameters dark_matter_params);
void track_total_baryons(Cosmology &cosmology, ExecutionParameters execparams, SimulationParameters simulation_params, const std::vector<HaloPtr> &halos,
</code_context>
<issue_to_address>
**suggestion:** Align declaration with recommended const-reference signature for DarkMatterHaloParameters
To stay consistent with the implementation, this parameter should be `const DarkMatterHaloParameters &dark_matter_params` instead of pass-by-value, avoiding an extra copy per snapshot and making the non-mutating intent explicit.
```suggestion
void transfer_galaxies_to_next_snapshot(const std::vector<HaloPtr> &halos, int snapshot, TotalBaryon &AllBaryons, const DarkMatterHaloParameters &dark_matter_params);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Minor changes to the mass swapping fixes when calculating galaxy properties
Mass swapping modifications applied to properties of type 2 galaxies
Corrections when assigning the spin parameter (with mass swapping fixes or without them)
Redefine host halo when we select the central subhalos, since the subhalo_type is being changed, but the host halo ID is not. This is leading to type 0 galaxies associated with satellite subhalos and type 1 galaxies associated with central subhalos when looking at the output variables id_halo_tree and id_subhalo_tree.
Additional minor changes: including cases where the final snapshot is different to z=0, removing unused parts and other bits
Include a conditional if statement when m_in<0 due to precision errors
Remove the Python script that generated the SFR-Mstars plane.
Summary by Sourcery
Adjust dark matter mass-swapping handling and related galaxy/subhalo property propagation across tree building, halo evolution, and physical modules.
Bug Fixes:
Enhancements:
Documentation:
Chores: