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

Bug in Despali16 #1197

Merged
merged 3 commits into from
Oct 2, 2024
Merged

Bug in Despali16 #1197

merged 3 commits into from
Oct 2, 2024

Conversation

damonge
Copy link
Collaborator

@damonge damonge commented Sep 3, 2024

Closes #1195
This bug was not caught by our tests because it would only show up when the chosen mass definition differs from Delta_vir.

@damonge
Copy link
Collaborator Author

damonge commented Sep 3, 2024

I've added a benchmark test that would have caught this bug. In doing so I regenerated all our benchmark files for halo mass function and halo biases using the latest version of colossus. The changes do not affect our benchmarks, which remain sufficiently accurate.

@damonge
Copy link
Collaborator Author

damonge commented Sep 3, 2024

Unrelated to this, there have been modifications to velocileptors which required me to update those benchmarks too.

Sorry for the many file modifications. Most of them are benchmark data, so any review should only focus on the code modifications (which are very small).

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10681318177

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.464%

Totals Coverage Status
Change from base Build 9990842930: 0.0%
Covered Lines: 6611
Relevant Lines: 6783

💛 - Coveralls

@combet combet self-requested a review October 2, 2024 13:55
Copy link

@combet combet left a comment

Choose a reason for hiding this comment

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

I've looked at the changes in the code, cross-checking the coefficients with the Despali paper and the fix looks good. Also good to have an additional benchmark to catch anything going wrong if the mass definition is not in Delta_vir.

@damonge damonge merged commit ca1b026 into master Oct 2, 2024
4 checks passed
@damonge damonge deleted the despali_hmf_fix branch October 2, 2024 15:08
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.

Typo in the despali.16.py file
3 participants