Update Neutron Star Section by Zhengjingyi0823 · Pull Request #425 · LSST-strong-lensing/slsim

Zhengjingyi0823 · 2026-05-19T16:07:05Z

Adding function for Neutron stars.

codecov · 2026-05-19T16:39:45Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.61%. Comparing base (7fa85b4) to head (a17debf).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #425      +/-   ##
==========================================
- Coverage   98.61%   98.61%   -0.01%     
==========================================
  Files         104      106       +2     
  Lines        8030     8085      +55     
==========================================
+ Hits         7919     7973      +54     
- Misses        111      112       +1

Files with missing lines	Coverage Δ
slsim/Sources/Events/BNSMerger/BNSMerger_pop.py	`100.00% <100.00%> (ø)`
slsim/Sources/Events/event_pop.py	`100.00% <100.00%> (ø)`
slsim/Sources/Supernovae/supernovae_pop.py	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sibirrer

thank you very much @Zhengjingyi0823! I made some comments and requests to further polish documentation and code

sibirrer · 2026-05-24T00:30:14Z

+        else:
+            raise ValueError("model should be chosen from 'BNS' or 'SNIa'")
+
+    def calculate_event_rate(self, z):


please precisely document the specific return with units

I have added the description of the outputs of functions and their units.

this should be in the docstrings of this definition, not the init statement

sibirrer · 2026-05-24T00:30:42Z

+            raise ValueError("model should be chosen from 'BNS' or 'SNIa'")
+
+    def calculate_event_rate(self, z):
+        if self.model_name == "BNS":


if you have the definitions of the two separate classes named the same, you don't need an if/else statement here

I added a wrapper function called calculate_event_rate in supernovae_pop.py so that we don’t need to change old code which calls the calculate_SNIa_rate and can simplify code in event_pop.py at the same time.

sibirrer · 2026-05-24T00:32:02Z

+        ft_d = 1 / (t_d * (np.log(self.t_d_max / self.t_d_min)))
+        return ft_d
+
+    def z_from_time(self, t):


this is a very generic definition that deserves to be at a different place where it can be re-used. This routine is not specific of what the BNSMerger_pop class is about

I moved delay_time_destribution out of the class BNSMergerRate. Since it is normalized, I renamed it as norm_delay_time_distribution.

this does not addresses your comment. This comment was about z_from_time(t)

sibirrer · 2026-05-24T00:38:27Z

+        self.t_d_max = 13.8  # Gyr, Hubble time approximately
+
+    def calculate_star_formation_rate(self, z):
+        """Calculates the binary formation rate. (Eq 3 - Kuwahara et al. 2025)


is this star formation or binary formation?

I originally kept the name for consistency with the supernova population code. Since this function is mainly used within the BNS merger model and only externally called in the BNS merger notebook, I renamed it to calculate_binary_formation_rate for clarity.

sibirrer · 2026-05-24T00:38:55Z

+        """Calculates the binary formation rate. (Eq 3 - Kuwahara et al. 2025)
+
+        :param z: redshift (z>=0)
+        :param z_m: peak-related redshift parameter


this does not seem to be an input

I moved the fixed model parameters nu, a, b, and z_m out of the :param: section and rewrote their explanation in the correct format.

sibirrer · 2026-05-24T00:40:46Z

+
+        :param t_d: time delay (t_d>=0) in [Gyr]
+        :param t_d_min: minimum of t_d in [Gyr], assumed as 20 Myr
+        :param t_d_max: maximum of t_d in [Gyr], assumed as Hubble time


this is not an input here. I would make this either a default-valued input or better document it in the class initialization what it means

sibirrer · 2026-05-24T00:41:40Z

+        (Eq 4 - Kuwahara et al. 2025)
+
+        :param t_d: time delay (t>=0)
+        :param t: time at final integrated redshift (t>=0)


is there a condition that t>t_d? or something like that?

The condition t_d < t is already enforced by the integration range in calculate_event_rate. To make this clearer, I added this condition as a part of comment above the code.

sibirrer · 2026-05-24T00:41:54Z

+
+        :param z: redshift (z>=0)
+
+        :return: BNS merger rate R_m(z), following the unit of R_f


what units?

The specific unit is added.

sibirrer · 2026-05-24T00:42:47Z

+    def calculate_event_rate(self, z):
+        """Calculates the rate of events, such as BNS merger. (Eq 4 - Kuwahara et al. 2025)
+
+        :param z: redshift (z>=0)


this is an array of redshifts, please specify. Do they need to be ascending/decending etc?

I completed the description of z in function calculate_event_rate. Array z don't need to be sorted but in practice use of BNS merger model, it should be descending since it is converted from t by function z_from_time with the time-delay order from t_d_min to t_d_max.

sibirrer · 2026-05-24T00:44:56Z

+
+        rate_array = event_pop.calculate_event_rate(self.z_array)
+
+        npt.assert_almost_equal(rate_array[0], 0.03081, decimal=3)


here I would test against the specific class instance's routine (which should be the same) instead of against a numerical answer (that is a test for the individual classes test function

I have modified test_event_pop.py to compare the output of EventPopulation with the corresponding specific population model class.

…tion related to Event, supernova and BNSmerger

sibirrer

Thank you @Zhengjingyi0823 ! I still have some comments that should be addressed

sibirrer · 2026-05-26T17:19:17Z

+        ft_d = 1 / (t_d * (np.log(self.t_d_max / self.t_d_min)))
+        return ft_d
+
+    def z_from_time(self, t):


this does not addresses your comment. This comment was about z_from_time(t)

sibirrer · 2026-05-26T17:21:17Z

+    def __init__(self, model, cosmo, z_max):
+        """
+        BNS model calls the function to calculate the rate of BNS merger. (Eq 4 - Kuwahara et al. 2025)
+        :return: BNS merger rate R_m(z), in (M_sol)yr^(-1)Gpc^(-3)


initialization do not have any returns.
You should make the two definitions to provide the same units. It's very dangerous to have different returns for different settings

sibirrer · 2026-05-26T17:21:54Z

+        else:
+            raise ValueError("model should be chosen from 'BNS' or 'SNIa'")
+
+    def calculate_event_rate(self, z):


this should be in the docstrings of this definition, not the init statement

Zhengjingyi0823 and others added 2 commits May 19, 2026 12:01

Updated event_pop.py

dee627e

Autofix formatting from pre-commit.com hooks

5fb48be

Zhengjingyi0823 and others added 4 commits May 22, 2026 22:20

Submit BNSMerger_pop.py, event_pop.py, and their test file.

4c42740

Autofix formatting from pre-commit.com hooks

429a680

Add the missing test for wrong input model name in test_event_pop.py

5305c3a

Autofix formatting from pre-commit.com hooks

9c25ece

sibirrer requested changes May 24, 2026

View reviewed changes

Zhengjingyi0823 and others added 2 commits May 25, 2026 23:33

Modified documents, description, function, improved function organiza…

c8bf214

…tion related to Event, supernova and BNSmerger

Autofix formatting from pre-commit.com hooks

a17debf

Zhengjingyi0823 requested a review from sibirrer May 26, 2026 04:10

sibirrer requested changes May 26, 2026

View reviewed changes


		:param z: redshift (z>=0)

		:return: BNS merger rate R_m(z), following the unit of R_f


		rate_array = event_pop.calculate_event_rate(self.z_array)

		npt.assert_almost_equal(rate_array[0], 0.03081, decimal=3)

Conversation

Zhengjingyi0823 commented May 19, 2026

Uh oh!

codecov Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sibirrer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sibirrer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented May 19, 2026 •

edited

Loading