[fix] Corrected/enhanced merger process by ezapartas · Pull Request #785 · POSYDON-code/POSYDON

ezapartas · 2026-01-15T14:39:02Z

Implementing and solving issues already discussed in a previous PR version #311 which is now deprecated.

No deepcopy during merger (which was working, but led to memory and speed issues)
However, correct caclulations of the mass weighted averages of abundances BEFORE mass changes.
calculating center_gamma and avg_c_in_c_core for mergers whenever possible (as the core value if the cores do not merge and np.nap and mass-weighted average respectively, if the cores merge).
added co_core_mass as a potential option in the matching criteria at track_match (mostly for mergers)

Reminder : The resulting merger product is continuing as a singlestar instance at the place of the engulfing star (i.e. if oMerging1, then star_1 would be the merged product) UNLESS a NS/BH is involved, where the latter is the only one surviving (potential Thorne-Zytkov object).

…ighted averages

…ey only come from the core of the postMS star

…s (with changes) and the star_base (the HMS, star 1 probably) becomes massless remnant

…track_match

for more information, see https://pre-commit.ci

astro-abhishek · 2026-01-29T16:12:45Z

posydon/binary_evol/DT/step_merged.py

+            try:
+            	mass_weighted_avg_value=(A1*M1+A2*M2)/(M1+M2)
+            except TypeError:
+            	mass_weighted_avg_value= np.nan


Is setting the mass weighted average to np.nan going to cause issues later down the pipeline?

Is it possible to avoid a try/except?
Is it instead possible to verify the type of the values used?

@astro-abhishek the np.nan may cause issues later, but I think is the most pythonic way of saying "the function was called byt physically there are issues at the calculation so we have no computed result, we don't know the value". If the question is whether it will make it crash, I am not sure, I think not, as np.nan is accepted as a value in calculations, it will just produce more "unknown" np.nan quantities wherever it is used in a calcualation.

@maxbriel The following is a different way more exact checking, instead of try-except loop. Is this what you are thinking? If did not add it yet, but if you agree, please just add it as a commit directly.

den = M1 + M2
if not (np.isfinite(A1) and np.isfinite(A2) and np.isfinite(M1) and np.isfinite(M2)):
mass_weighted_avg_value = np.nan
elif den == 0:
mass_weighted_avg_value = np.nan
else:
mass_weighted_avg_value = (A1M1 + A2M2) / den

In both cases, if you have better coding ideas for this part (or any other) feel free to add them directly.

posydon/binary_evol/DT/step_merged.py

astro-abhishek · 2026-01-29T16:18:46Z

posydon/binary_evol/DT/track_match.py

                "log_R",
-                "center_c12"
+                "center_c12",
+                "co_core_mass"


what if the co_core_mass doesn't exist? (there's a comment about something similar right above this)

Hmm, this list assumes that the single star models and the respective EEPs have this column. If a user uses their one sinlge star and EEPs, not run with POSYDON, so not calculating a co_core (having only a c_core or an o_core for example), there may be a mismatch. For our POSYDON runs this is ok for now. If we want to generalize it so much, I think much of the structure should change anyway. I would leave it for now, keeping the above comment there for future reference.

maxbriel · 2026-02-09T15:12:55Z

There are two testing binaries in the suite that change with this PR.
@ezapartas can you confirm that these changes are expected from the change in the PR?

maxbriel · 2026-02-10T05:47:01Z

posydon/binary_evol/DT/step_merged.py

+
                # weighted central abundances if merging cores. Else only from star_base
-                if (comp.co_core_mass > 0 and star_base.co_core_mass == 0): # comp with CO core and the star_base has not
+                if (comp.co_core_mass is not None and star_base.co_core_mass == 0): # comp with CO core and the star_base has not


Why is there this change from values to None? It happens later too.

Are there specific binaries that crash because the co_core_mass is undefined when reaching this stage?

Same n line 575.

Hmm, I don' remember exactly the reasoning. I think I thought that instead of raising a TypeError in case of comp.co_core_mass = None and crashing, I preferred it to go to the else: Pwarn("weird compbination of CO core masses during merging", "EvolutionWarning").

But again, if you think of a better handling it , feel free to directly change it back

posydon/binary_evol/DT/step_merged.py

maxbriel · 2026-02-10T05:51:28Z

posydon/binary_evol/DT/step_merged.py

is this TODO still needed?

No, you are right, I took the line out

posydon/binary_evol/DT/step_merged.py

maxbriel · 2026-02-10T05:56:09Z

posydon/binary_evol/DT/step_merged.py

-                for key in ["mass", "he_core_mass", "c_core_mass", "o_core_mass", "co_core_mass"]:
-                    current = getattr(star_base, key) + getattr(comp, key)
-                    setattr(merged_star, key,current)
+                merged_star = comp


Why are we changing what the merged star is here?

This is different than the definition of which one starts the RLO. This will make the companion the remaining star.

Hmm, actually this occurs even at line 256. Indeed, it "violates" the engulfind star (that starts the RLO) is our base of the merger, making the base to be the star with the most layers. So for example in a postMS+HMS merger, the postMS star becomes the base always. Same between postMS+HeMS. Even if it is not the postMS that engulfs. This is because the final merged star with have a more complex layering (envelope, core, etc). Of course in most cases it will indeed be the postMS star that is also initiating the RLO and the engulfing, but we allow for the other case too, giving the same outcome.

If needed we can restructure the code to remake the calculation using the engulfing always as the base, but this is only for the sake of clarity. It will not change the merged outcome in the end.

summary: The outcome in the end should not depend on which star engulfs (unless omeone argues otherwise). it is just easier to start the merger construction from that star in most cases, as it usually is the more "complex" star in the perspective of layers, i.e. it is a giant star

posydon/binary_evol/DT/step_merged.py

ezapartas · 2026-02-12T10:34:25Z

@astro-abhishek @maxbriel Thanks for the detailed comments. I tried to answer all of them and address whichever where doable. Feel free to directly change the coding way if you think a better option exists.

Note as soon as this PR is accepted, it is better to be pulled by the #788 before that PR is evaluated and merged.

astroJeff · 2026-02-12T15:28:59Z

posydon/binary_evol/DT/step_merged.py

+                    merged_star.avg_c_in_c_core = mass_weighted_avg(abundance_name = "avg_c_in_c_core", mass_weight1="he_core_mass")
                    merged_star.center_n14 = mass_weighted_avg(abundance_name = "center_n14", mass_weight1="he_core_mass")
                    merged_star.center_o16 = mass_weighted_avg(abundance_name = "center_o16", mass_weight1="he_core_mass")
+                    setattr(merged_star, "center_gamma", np.nan)


Can we not use the parameters of the helium stars to set center_gamma?

astroJeff · 2026-02-12T15:29:35Z

posydon/binary_evol/DT/step_merged.py

+                    merged_star.avg_c_in_c_core = mass_weighted_avg(abundance_name = "avg_c_in_c_core", mass_weight1="co_core_mass")
                    merged_star.center_n14 = mass_weighted_avg(abundance_name = "center_n14", mass_weight1="co_core_mass")
                    merged_star.center_o16 = mass_weighted_avg(abundance_name = "center_o16", mass_weight1="co_core_mass")
+                    setattr(merged_star, "center_gamma", np.nan)


Same comment as above about center_gamma

astroJeff · 2026-02-12T15:30:11Z

posydon/binary_evol/DT/step_merged.py

+                    merged_star.avg_c_in_c_core = mass_weighted_avg(abundance_name = "avg_c_in_c_core", mass_weight1="he_core_mass")
                    merged_star.center_n14 = mass_weighted_avg(abundance_name = "center_n14", mass_weight1="he_core_mass")
                    merged_star.center_o16 = mass_weighted_avg(abundance_name = "center_o16", mass_weight1="he_core_mass")
+                    setattr(merged_star, "center_gamma", np.nan)


Same comment as above. Why set to nan instead of grabbing the value from either star_base or comp?

astroJeff · 2026-02-12T15:30:25Z

posydon/binary_evol/DT/step_merged.py

+                    merged_star.avg_c_in_c_core = mass_weighted_avg(abundance_name = "avg_c_in_c_core", mass_weight1="he_core_mass")
                    merged_star.center_n14 = mass_weighted_avg(abundance_name = "center_n14", mass_weight1="he_core_mass")
                    merged_star.center_o16 = mass_weighted_avg(abundance_name = "center_o16", mass_weight1="he_core_mass")
+                    setattr(merged_star, "center_gamma", np.nan)


astroJeff · 2026-02-12T15:30:37Z

posydon/binary_evol/DT/step_merged.py

+                    merged_star.avg_c_in_c_core = mass_weighted_avg(abundance_name = "avg_c_in_c_core", mass_weight1="co_core_mass")
                    merged_star.center_n14 = mass_weighted_avg(abundance_name = "center_n14", mass_weight1="co_core_mass")
                    merged_star.center_o16 = mass_weighted_avg(abundance_name = "center_o16", mass_weight1="co_core_mass")
+                    setattr(merged_star, "center_gamma", np.nan)


astroJeff · 2026-02-12T15:30:43Z

posydon/binary_evol/DT/step_merged.py

+                    merged_star.avg_c_in_c_core = mass_weighted_avg(abundance_name = "avg_c_in_c_core", mass_weight1="he_core_mass")
                    merged_star.center_n14 = mass_weighted_avg(abundance_name = "center_n14", mass_weight1="he_core_mass")
                    merged_star.center_o16 = mass_weighted_avg(abundance_name = "center_o16", mass_weight1="he_core_mass")
+                    setattr(merged_star, "center_gamma", np.nan)


astroJeff · 2026-02-12T15:30:58Z

posydon/binary_evol/DT/step_merged.py

+                    merged_star.avg_c_in_c_core = mass_weighted_avg(abundance_name = "avg_c_in_c_core", mass_weight1="he_core_mass")
                    merged_star.center_n14 = mass_weighted_avg(abundance_name = "center_n14", mass_weight1="he_core_mass")
                    merged_star.center_o16 = mass_weighted_avg(abundance_name = "center_o16", mass_weight1="he_core_mass")
+                    setattr(merged_star, "center_gamma", np.nan)


astroJeff · 2026-02-12T15:31:04Z

posydon/binary_evol/DT/step_merged.py

+                    merged_star.avg_c_in_c_core = mass_weighted_avg(abundance_name = "avg_c_in_c_core", mass_weight1="co_core_mass")
                    merged_star.center_n14 = mass_weighted_avg(abundance_name = "center_n14", mass_weight1="co_core_mass")
                    merged_star.center_o16 = mass_weighted_avg(abundance_name = "center_o16", mass_weight1="co_core_mass")
+                    setattr(merged_star, "center_gamma", np.nan)


astroJeff · 2026-02-12T15:31:51Z

Can we spin off some of this into new functions? There is quite a bit of repeated code

ezapartas added 10 commits January 15, 2026 13:34

started a new branch and some pint statements

617bd07

started process for HMS+HMS: the mass change occurs after the mass we…

b057d47

…ighted averages

in postMS+MS merger, some central attributes do not become np.nan, th…

253fc72

…ey only come from the core of the postMS star

no working with HMS+postMS mergers, where the merger is the companion…

c0c41f3

…s (with changes) and the star_base (the HMS, star 1 probably) becomes massless remnant

also not erasing core properties

a8ae476

finished postMS+He stars and opposite

bba592a

now finished (issue with which star became massless)

ed37883

finished until the beginning of He+He

e434db5

finished step_merged

5aa5602

added co_core_mass as a potential option in the matching criteria at …

12f97e5

…track_match

ezapartas requested review from maxbriel and sgossage and removed request for sgossage January 15, 2026 14:39

[pre-commit.ci] auto fixes from pre-commit.com hooks

37b64b6

for more information, see https://pre-commit.ci

ezapartas requested review from dimsour94 and sgossage January 15, 2026 14:39

astroJeff requested a review from astro-abhishek January 22, 2026 16:01

ezapartas mentioned this pull request Jan 28, 2026

[enhancement] HMS+HMS Mergers rejuvenation according to Schneider+2016 #788

Open

astro-abhishek reviewed Jan 29, 2026

View reviewed changes

posydon/binary_evol/DT/step_merged.py Outdated Show resolved Hide resolved

astro-abhishek reviewed Jan 29, 2026

View reviewed changes

maxbriel changed the title ~~Corrected/enhanced merger process~~ [fix] Corrected/enhanced merger process Feb 9, 2026

maxbriel reviewed Feb 10, 2026

View reviewed changes

maxbriel added enhancement New feature or request bug Something isn't working labels Feb 10, 2026

ezapartas added 3 commits February 12, 2026 11:07

removed old code line

dd7078a

ZeroDivisionError added

a5c4a85

deleted a TODO comment

68a57e0

astroJeff reviewed Feb 12, 2026

View reviewed changes

Conversation

ezapartas commented Jan 15, 2026

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxbriel commented Feb 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezapartas Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ezapartas commented Feb 12, 2026

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!

astroJeff commented Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

ezapartas Feb 12, 2026 •

edited

Loading