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

Merger trees artefact treatment: mass swapping and massive transients #19

Merged
merged 14 commits into from
Aug 14, 2024

Conversation

angel-chandro
Copy link
Contributor

@angel-chandro angel-chandro commented Jul 30, 2024

  1. Redundancy in define_central_subhalo for mass swapping fix
    Subhalo properties only redefined with the host halo properties for central subhalos when applying mass swapping fixes. Halos vvir and concentration defined with host halo properties and after ensuring their mass growth

  2. Massive transients implementations
    Input file with a list of the main branch subhalos that are classified as transients and the halos that host them. When building the trees and making connections subhalo-descendant, if there is a transient in the descendant halo and several constraints are fulfilled, then default subhalo-descendant connection is broken and a subhalo-transient connection has priority. These criteria can be summarized as: 1. transient more massive than descendant, 2. subhalo-descendant connection has a severe mass loss, 3. subhalo-transient connection is smooth enough in terms of the mass. Define parameters used for the massive transients treatment and for computing statistics. Set to true the default flags to apply mass swapping and massive transient fixes.
    After playing with the parameters involved in the transient fixes, we set their default values considering mass changes more or less smooth: the mass ratio to break a main progenitor-subhalo link should be <0.7 while the mass ratio to create a new main progenitor-transient link should be in the range 0.1-3.

Summary by Sourcery

This pull request introduces fixes for mass swapping in central subhalos and implements a new treatment for massive transients. It includes new parameters for controlling these fixes and enhances the tree building process to prioritize subhalo-transient connections under certain conditions. Additionally, it updates the execution configuration and logging to support these new features.

  • New Features:
    • Implemented mass swapping fixes for central subhalos, ensuring their properties are redefined with host halo properties.
    • Introduced massive transient treatment, allowing subhalo-transient connections based on specific criteria to improve tree building and connection accuracy.
  • Enhancements:
    • Added parameters for massive transient treatment, including flags and mass ratio thresholds, to the execution configuration.
    • Updated the tree building process to include statistics for massive transient events, providing detailed logging of the fixes applied.

angel-chandro and others added 4 commits July 18, 2024 13:51
Subhalo properties only redefined with the host halo properties for central subhalos when applying mass swapping fixes. Halos vvir and concentration defined with host halo properties and after ensuring their mass growth
Input file with a list of the subhalos that are classified as transients and the halos that host them. When building the trees and making connections subhalo-descendant, if there is a transient in the descendant halo and several constraints are fulfilled, then default subhalo-descendant connection is broken and a subhalo-transient connection has priority. These criteria can be summarized as: 1. transient more massive than descendant, 2. subhalo-descendant connection has a severe mass loss, 3. subhalo-transient connection is smooth enough in terms of the mass. Define parameters used for the massive transients treatment and for computing statistics. Set to true the default flags to apply mass swapping anf massive transient fixes
Final changes to implement the massive transient treatment. Plus after playing with the parameters involved in the transient fixes, we set their default values considering mass changes more or less smooth: the mass ratio to break a main progenitor-subhalo link should be <0.7 while the mass ratio to create a new main progenitor-transient link should be in the range 0.1-3.
Copy link

sourcery-ai bot commented Jul 30, 2024

Reviewer's Guide by Sourcery

This pull request introduces fixes for mass swapping and implements massive transient treatments. The changes include updating the tree building logic to handle massive transient events, adding configuration parameters for these treatments, and modifying the Subhalo and Halo classes to include transient-related attributes.

File-Level Changes

Files Changes
src/tree_builder.cpp
include/tree_builder.h
Updated tree building logic to handle massive transient events and apply mass swapping fixes.
src/merger_tree_reader.cpp
include/halo.h
Added logic to read and set transient flags for halos.
include/execution.h
src/execution.cpp
sample_lagos23.cfg
Added and loaded configuration parameters for massive transients treatment.
include/subhalo.h
include/halo.h
Added transient-related attributes to Subhalo and Halo classes.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

@angel-chandro angel-chandro changed the title Numerical artefact treatment: mass swapping and massive treatments Merger trees artefact treatment: mass swapping and massive treatments Jul 30, 2024
@angel-chandro angel-chandro changed the title Merger trees artefact treatment: mass swapping and massive treatments Merger trees artefact treatment: mass swapping and massive transients Jul 30, 2024
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @angel-chandro - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

src/tree_builder.cpp Show resolved Hide resolved
src/tree_builder.cpp Show resolved Hide resolved
angel-chandro and others added 10 commits August 1, 2024 12:11
Massive transient catalogues are not required right noe. The massive transients are flagged inside the code as the subhalos that belong to main branches that are born with an unexpectedly high number of particles: 1. find the subhalos with no main progenitor, 2. find subhalos with no progenitor that belong to main branches, 3. they are defined as transients if the particle number at birth is above the considered threshold. There are 3 different threshold in the particle number at birth for main branches: a constant value of 200 particles, a constant value of 10 times the minimum particle number for a subhalo, and 3 times the standard deviation of the particle number at birth for main branches at each snapshot. Set as a default the 3 sigma method.
Default transient definition: subhalos that belong to main branches whose particle number at birth is above 10 times the minimum particle number for a structure
The logic was slightly wrong, meaning we found ourselves against a
potential null pointer dereference.

Signed-off-by: Rodrigo Tobar <[email protected]>
Signed-off-by: Rodrigo Tobar <[email protected]>
Signed-off-by: Rodrigo Tobar <[email protected]>
@rtobar rtobar merged commit ce7cac5 into ICRAR:devel Aug 14, 2024
8 checks passed
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