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

Origin/trees issues fixes #16

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Origin/trees issues fixes #16

merged 3 commits into from
Jun 27, 2024

Conversation

cdplagos
Copy link
Collaborator

@cdplagos cdplagos commented Jun 27, 2024

Summary by Sourcery

This pull request introduces enhancements to the handling of subhalo properties by adding conditional logic based on a new parameter 'apply_fix_to_mass_swapping_events'. It refactors existing functions to separate the handling of central and satellite subhalos and adds a new function for recalculating angular momentum.

  • Enhancements:
    • Introduced conditional logic to redefine central and satellite subhalo properties based on the new parameter 'apply_fix_to_mass_swapping_events'.
    • Refactored the function 'define_properties_halos' into two separate functions: 'define_properties_central_subhalos' and 'define_properties_satellite_subhalos'.
    • Added a new function 'redefine_angular_momentum' to handle the recalculation of angular momentum for subhalos.
    • Updated various functions to use the new 'apply_fix_to_mass_swapping_events' parameter to conditionally apply fixes to mass swapping events.

cdplagos added 3 commits June 27, 2024 12:06
- separated the function to redefine subhalo properties into centrals
and satellites. This is done because the one for centrals needs to be
called before the function that defines ages, while the satellite one
needs to be called afterwards.

- Fixed the calculation for satellite subhalos to use the infall
redshift rather than the current redshift (given that it was using all
the properties at infall).
This leads to many changes to make sure one can run the previous model
without the fixing of mass swapping events (to help with
reproduceability).

Another change is to make sure the angular momentum vector L of subhalos
is redefine in the cases where lambda_random is used.
I also changed added a bool input to evolve_galaxy to indicate whether
we want to apply the fix for the mass swapping cases.
Copy link

sourcery-ai bot commented Jun 27, 2024

Reviewer's Guide by Sourcery

This pull request addresses issues related to mass swapping events in the tree building and dark matter halo processes. It introduces a new parameter, apply_fix_to_mass_swapping_events, to conditionally apply fixes to subhalo properties. Functions have been refactored to handle central and satellite subhalo properties separately, and new functions have been added to redefine subhalo properties based on the new parameter. Additionally, minor typos and indentation issues have been fixed across multiple files.

File-Level Changes

Files Changes
src/tree_builder.cpp
include/tree_builder.h
Refactored functions to handle central and satellite subhalo properties separately and added conditional checks for mass swapping events.
src/dark_matter_halos.cpp
include/dark_matter_halos.h
Introduced apply_fix_to_mass_swapping_events parameter and related logic to handle subhalo properties more accurately.
src/shark_runner.cpp
src/disk_instability.cpp
src/galaxy_mergers.cpp
include/disk_instability.h
include/galaxy_mergers.h
Updated constructors and function calls to pass dark_matter_params and handle mass swapping events.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • 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.

@cdplagos cdplagos merged commit f7cb4b8 into devel Jun 27, 2024
15 of 19 checks passed
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 @cdplagos - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 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.

@rtobar rtobar deleted the origin/trees-issues-fixes branch July 4, 2024 13:20
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.

1 participant