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 fix: Adding min and max tau values for inverse td_model in p_tau function #5046

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

divyajyoti09
Copy link
Contributor

This change introduces a minimum and maximum tau for the inverse model in pycbc.population.population_models.p_tau() and fixes this issue: #5043

Standard information about the request

This is a: bug fix

This change affects: PyCBC population models module

This change changes: scientific output

This change has been tested at the level of creating merger_rate_density plots.

Motivation

This change is made because the current state of the inverse model in the p_tau function is practically non-functional as it doesn't include any time delay. This has been demonstrated in the issue here: #5043

Links to any issues or associated PRs

#5043

Testing performed

This code has been tested to generate plots for merger_rate_density function

  • The author of this pull request confirms they will adhere to the code of conduct

@divyajyoti09 divyajyoti09 changed the title Adding min and max tau values for inverse td_model in p_tau function Bug fix: Adding min and max tau values for inverse td_model in p_tau function Feb 17, 2025
@ahnitz ahnitz requested a review from WuShichao February 17, 2025 16:53
@WuShichao
Copy link
Contributor

Thank you for this PR, I will review it this week.

@WuShichao
Copy link
Contributor

WuShichao commented Feb 17, 2025

Can you show the new curve for the inverse time delay model? I noticed in the issue #5043 that you normalized those curves. Actually, SFR itself has a different unit (solar-mass Mpc^-3 yr^-1 ) than the curve convoluted by the time delay model. Have you tried the power_law delay model? The inverse is just a special case of it. Previously, I knew the shape of the inverse model should be the same as SFR.

@divyajyoti09
Copy link
Contributor Author

@WuShichao The new plot looks like this
Screenshot from 2025-02-18 12-46-52

I agree that the units are different for SFR and merger rate but I normalized them to see whether the time delay was introducing a shift in the peak (which it wasn't because all the probabilities were peaking at 0 time delay). After introducing the td_min limit, the curve is shifting as expected.

Also, although I agree that power_law is very close to the inverse model, I still think this bug in the inverse model should be fixed since it is being used as default in various functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants