Trees issues fixes #15
Closed
Conversation
This file contains 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
These were producing warnings, and thus prevented us from turning on -Werror on CI, which is something we'd like to have. I checked with Claudia, and she said it was safe to remove these. Signed-off-by: Rodrigo Tobar <[email protected]>
This was issuing a warning, so let's get rid of it. Signed-off-by: Rodrigo Tobar <[email protected]>
Also make sure our CI jobs check that it doesn't go out of sync. Signed-off-by: Rodrigo Tobar <[email protected]>
The latter has been deprecated since numpy 1.6, and effectively removed in the latest versions of numpy. The "density" argument has equivalent semantics, and the documentation states that it should be used instead. Signed-off-by: Rodrigo Tobar <[email protected]>
This flag contains the *space-separated* flags that are needed for compiling OpenMP programs. This is not the format expected by add_compile_options though, so we need to turn the string into a list before passing it on. Signed-off-by: Rodrigo Tobar <[email protected]>
There were a few things that were not correctly done around OpenMP checks that are being corrected here, namely: * Change DECLARED -> DEFINED, since the former doesn't mean anything (so this code has been always wrong). * Change OPENMP_FOUND -> OpenMP_FOUND, with the latter being the more generic way of checking the result of find_package. * Use the correct casing for the rest of OpenMP* variable, which were previously uppercased in our code. * Add a check for the OpenMP_CXX_VERSION variable in particular, which might be set while OpenMP_VERSION might not. Signed-off-by: Rodrigo Tobar <[email protected]>
This new port now tries to *not* rely too much on GitHub Actions conventions (which are not that many, anyway), and also splits the big test.sh script we used with Travis into smaller scripts in a new .ci directory that can be invoked separately, therefore allowing us to model each individual test as a different step on the GitHub Actions workflow. Signed-off-by: Rodrigo Tobar <[email protected]>
Signed-off-by: Rodrigo Tobar <[email protected]>
The new file had been added to the tree, but wasn't included anywhere in the documentation, duh! Signed-off-by: Rodrigo Tobar <[email protected]>
The implicit build.image is deprecated, so let's use the new build.os key explicitly instead. Signed-off-by: Rodrigo Tobar <[email protected]>
Signed-off-by: Rodrigo Tobar <[email protected]>
Signed-off-by: Rodrigo Tobar <[email protected]>
We used to depend on >= 1.54, but in a809629 we accidentally raised the minimum required Boost version to 1.68 (boost::optional::has_value() was introduced in that version to align with std::optional). This version of Boost is 5 years old though, so instead of modifying our code to cope with older versions, we're probably better off declaring a higher minimum version. This was the original cause of #13, and thus it should prevent this issue from happening again in the near future. Signed-off-by: Rodrigo Tobar <[email protected]>
Signed-off-by: Rodrigo Tobar <[email protected]>
Signed-off-by: Rodrigo Tobar <[email protected]>
The option defaults to false, and indicates whether the lognormal distribution used to draw lambda values should use a fix value of 0.03, or a dynamic value based on the halo spin distribution following Kim et al. (2015), as its m parameter (the natural logarithm of the distribution's mean). The Lagos et al. (2023) paper used this dependency, and therefore we're also updating the sample configuration file that reflects that paper to ensure reproducibility. Signed-off-by: Rodrigo Tobar <[email protected]>
Signed-off-by: Rodrigo Tobar <[email protected]>
…current properties or at infall for satellites
…ent properties or at infall for satellites. Massive transients: read catalogue with halos affected by this
Signed-off-by: Rodrigo Tobar <[email protected]>
- 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.
Reviewer's Guide by SourceryThis pull request addresses several issues related to the handling of subhalos in the 'Trees' module. The changes include fixing the calculation for satellite subhalos to use the redshift at infall, separating functions for central and satellite subhalos, adding an input parameter to control mass swapping event fixes, and redefining the angular momentum vector of subhalos when using lambda_random. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cdplagos - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||
| run: | | ||
| sudo apt install libhdf5-dev hdf5-tools libboost-filesystem-dev libboost-program-options-dev libboost-log-dev cxxtest libgsl-dev | ||
| sudo apt install ninja-build libhdf5-dev hdf5-tools libboost-filesystem-dev libboost-program-options-dev libboost-log-dev cxxtest libgsl-dev |
There was a problem hiding this comment.
suggestion: Consider adding -y flag to apt install
Adding the -y flag to apt install can help automate the installation process by automatically agreeing to the installation of packages.
Suggested change
| sudo apt install ninja-build libhdf5-dev hdf5-tools libboost-filesystem-dev libboost-program-options-dev libboost-log-dev cxxtest libgsl-dev | |
| sudo apt install -y ninja-build libhdf5-dev hdf5-tools libboost-filesystem-dev libboost-program-options-dev libboost-log-dev cxxtest libgsl-dev |
| * ``bh_accretion_rate_hh_history``: Black hole accretion rate due to hot halo cooling [Msun/yr/h]. | ||
| * ``bh_accretion_rate_sb_history``: Black hole accretion rate due to starbursts [Msun/yr/h]. | ||
| * ``bh_spin``: Black hole spin history [dimensionless]. | ||
| * ``id_galaxy``: galaxy ID. Unique to this galaxy throughout time. If this galaxy never mergers onto a central, then its ID is always the same. |
There was a problem hiding this comment.
issue (documentation): Fix typo in documentation.
The word 'mergers' should be 'merges'.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Took code implemented by Angel Chandro and made several changes:
Summary by Sourcery
This pull request addresses several issues related to the handling of subhalo properties, particularly for satellite subhalos. It introduces new features to control mass swapping events, separates functions for central and satellite subhalos, and fixes the calculation of properties using redshift at infall. The CI workflow and build system have been enhanced to improve testing and documentation processes.