[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

                #these stellar attributes change value
-                merged_star.mass = (star_base.mass + comp.mass) * (1.-self.rel_mass_lost_HMS_HMS)
+
                #TODO for key in ["center_h1", "center_he4", "center_c12", "center_n14","center_o16"]:


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.

maxbriel · 2026-03-13T18:51:54Z

Here's the output of the branch currently:
manos merger copiedattributes

main

The diff: https://www.diffchecker.com/7IurVLUA/

posydon/binary_evol/DT/step_merged.py

sgossage · 2026-03-14T02:28:52Z

posydon/binary_evol/DT/step_merged.py

+
+        if self.verbose:
+            print(abundance_name, mass_weight1, mass_weight2)
+            print("A_base, M_base_abundance, A_comp, M_comp_abundance", A1, M1, A2, M2)


Should we say *_base or e.g., *_1 or *_pri? I think other parts of the code use one of the latter. Also, should M_*_abundance be something like M_*_mass because M1/M2 hold masses?

sgossage · 2026-03-14T02:33:27Z

posydon/binary_evol/DT/step_merged.py

+                # companion with the envelope of star_base
+                parameters_to_mix = ["surface_h1", "surface_he4", "surface_c12", "surface_n14", "surface_o16"]
+                for abundance_name in parameters_to_mix:
+                    setattr(merged_star, abundance_name, self.mass_weighted_avg(star_base,
+                                                                                comp,
+                                                                                abundance_name=abundance_name,
+                                                                                mass_weight1="H-rich_envelope_mass",
+                                                                                mass_weight2="mass"))
+
+                # The change of masses occurs after the calculation of weighted averages
+                # Note that the he core mass is unchanged, which means that
+                # adding the mass changes the envelope mass only.
+                merged_star.mass = star_base.mass + comp.mass

-                for key in STARPROPERTIES:
-                    # these stellar attributes become np.nan
-                    for substring in ["log_", "lg_", "surf_", "conv_", "lambda", "profile"]:
-                        if (substring in key) :
-                            setattr(merged_star, key, np.nan)
-                        if key in [ "c12_c12", "center_gamma",
-                                   "avg_c_in_c_core", "total_moment_of_inertia", "spin"]:
-                            setattr(merged_star, key, np.nan)
+                # Set parameters that are not expected to be meaningful after a merger to np.nan
+                self._apply_nan_attributes(merged_star)


Some of this is repeated code in this if chain. I suggest generalizing this and creating a helper function for clarity/brevity that is called here and elsewhere, as appropriate.

working on this here: sg_merger_copiedattributes

posydon/binary_evol/DT/step_merged.py

sgossage · 2026-03-14T15:43:35Z

posydon/binary_evol/DT/step_merged.py

This line (binary.event = None) means that the print statements based on binary.event (e.g., if binary.event == 'oMerging1':) below never fire when self.verbose = True. Should this be set to None after the verbose print? Is this line needed?

I commented the binary.event = None line for now.

sgossage · 2026-03-20T23:39:32Z

posydon/binary_evol/DT/step_merged.py

-                                   "avg_c_in_c_core", "total_moment_of_inertia", "spin"]:
+                        if key in [ "c12_c12", "total_moment_of_inertia", "spin"]:
                            setattr(merged_star, key, np.nan)
                merged_star.state = check_state_of_star(merged_star, star_CO=False)  # TODO for sure this needs testing!


The comment says that this for sure needs to be tested -- has it been?

sgossage · 2026-03-21T05:42:53Z

I was working on generalizing the logic and creating functions to apply these merging rules and noticed something.

For post-HMS mergers, we have this logic (in step_merged.py):

                      ...
385                # star_base with CO core and the comp has a He core
386                elif (star_base.co_core_mass > 0 and comp.co_core_mass == 0):
387                    pass # the central abundances are kept as the ones of star_base
                      ...

whereas for HMS-HMS mergers, we do not apply the same rule. Should we?

The following binary evolution from our test suite contains an HMS-HMS merger where the base star has a non-zero CO core mass while the companion has no CO core.

star_1= SingleStar(**{'mass': 9.845907 , 'state': 'H-rich_Core_H_burning','metallicity':1,
            'natal_kick_array':  [0.0, 4.190728383757787, 1.1521129697118118, 5.015343794234789]})
star_2 = SingleStar(**{'mass': 9.611029, 'state': 'H-rich_Core_H_burning','metallicity':1,
                'natal_kick_array': [0.0, 0.0, 0.0, 0.0]})
binary = BinaryStar(star_1, star_2, **{'time': 0.0,  'state': 'detached',  'event': 'ZAMS',
                        'orbital_period':  3.820571,'eccentricity': 0.0}, properties = sim_prop)

(The branch that I'm working on the logic in is sg_merger_copiedattributes. After we sort this out, if the changes there look good and the logic looks correct, we could merge it in to this branch. I've tested it with the test suite, but noticed that this binary produces a different outcome compared to manos_merger_copiedattribues because of this issue and how I currently wrote the logic.)

maxbriel · 2026-03-22T09:35:26Z

I was working on generalizing the logic and creating functions to apply these merging rules and noticed something.

For post-HMS mergers, we have this logic (in step_merged.py):
                      ...
385                # star_base with CO core and the comp has a He core
386                elif (star_base.co_core_mass > 0 and comp.co_core_mass == 0):
387                    pass # the central abundances are kept as the ones of star_base
                      ...
whereas for HMS-HMS mergers, we do not apply the same rule. Should we?

The following binary evolution from our test suite contains an HMS-HMS merger where the base star has a non-zero CO core mass while the companion has no CO core.
star_1= SingleStar(**{'mass': 9.845907 , 'state': 'H-rich_Core_H_burning','metallicity':1,
            'natal_kick_array':  [0.0, 4.190728383757787, 1.1521129697118118, 5.015343794234789]})
star_2 = SingleStar(**{'mass': 9.611029, 'state': 'H-rich_Core_H_burning','metallicity':1,
                'natal_kick_array': [0.0, 0.0, 0.0, 0.0]})
binary = BinaryStar(star_1, star_2, **{'time': 0.0,  'state': 'detached',  'event': 'ZAMS',
                        'orbital_period':  3.820571,'eccentricity': 0.0}, properties = sim_prop)
(The branch that I'm working on the logic in is sg_merger_copiedattributes. After we sort this out, if the changes there look good and the logic looks correct, we could merge it in to this branch. I've tested it with the test suite, but noticed that this binary produces a different outcome compared to manos_merger_copiedattribues because of this issue and how I currently wrote the logic.)

This is strange. A HMS-HMS merger should not have a CO core or He core defined in either component, so the same rule shouldn't be applied.
It's concerning that something with a CO core is being put into the HMS-HMS category. A few clarifying questions: Is the stellar state being updated correctly during the evolution when entering the step? Is it still burning central hydrogen? And how large is the "CO core"?

sgossage · 2026-03-22T18:42:52Z

It's concerning that something with a CO core is being put into the HMS-HMS category. A few clarifying questions: Is the stellar state being updated correctly during the evolution when entering the step? Is it still burning central hydrogen? And how large is the "CO core"?

@maxbriel this is the debugging output prior to merging:

Before Merger:
_____________

M1 = 4.212749495765598
M2 = 10.95210897562295
he_core_mass1 = 0.6365034990860114
he_core_mass2 = 2.019788928314233
co_core_mass1 = 0.10058851260570133
co_core_mass2 = 0.0

star_1.center_h1 = 0.03464608832551165
star_2.center_h1 = 0.016979750448212434
star_1.center_he4 = 0.8990294555960494
star_2.center_he4 = 0.9691717446871981
star_1.center_c12 = 0.05079687439526389
star_2.center_c12 = 9.913536056802633e-05
star_1.surface_he4 = 0.3547279661505059
star_2.surface_he4 = 0.32219608838969765

Star 1 is classed as H-rich_Core_H-burning by our criteria (log_LH = 3.4, surface_h1 = 0.6). I was able to trace it back to IF interpolation giving the star the 0.1 Msol CO core in step_HMS_HMS. The masses and age from IF interpolation seem reasonable, and it is classed as unstable MT (reasonable for that region of the grid), but somehow it determines a non-zero CO core mass, which is not reflected by our unstable MT models in the grids.

In the grids, this system may lie near a boundary between unstable MT and unstable reverse MT. While the unstable MT models have no CO core, the unstable reverse MT models do. These should be in different interpolation classes though?

I noticed that it seems we only have an unstable_MT interpolation class, and no unstable_reverse_MT. Could this be related to the issue? (@philipp-rajah)

At q = 0.95, the closest neighbors to this system are in the red boxed region:

and at q = 0.99:

(The axes in the plot are showing you the linear values of Porb and mass, not log10 values).

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

maxbriel added 5 commits March 13, 2026 17:22

move set outside of default call, since it's shared

7f5db49

Merge branch 'main' into manos_merger_copiedattributes

82f531d

add np.nan instead of None as default output

3c39ecb

add np.nan

a6a5b4d

add additional checks for A1, A2, M1, and M2

f4433c5

sgossage reviewed Mar 14, 2026

View reviewed changes

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

sgossage reviewed Mar 14, 2026

View reviewed changes

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

sgossage reviewed Mar 14, 2026

View reviewed changes

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

sgossage reviewed Mar 14, 2026

View reviewed changes

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

sgossage reviewed Mar 14, 2026

View reviewed changes

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

sgossage reviewed Mar 14, 2026

View reviewed changes

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

sgossage reviewed Mar 14, 2026

View reviewed changes

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

sgossage reviewed Mar 14, 2026

View reviewed changes

sgossage assigned ezapartas Mar 16, 2026

sgossage added 8 commits March 20, 2026 11:15

Update posydon/binary_evol/DT/step_merged.py

4bfdd77

Update posydon/binary_evol/DT/step_merged.py

c1dfa19

Update posydon/binary_evol/DT/step_merged.py

5530ed7

Update posydon/binary_evol/DT/step_merged.py

10af496

Update posydon/binary_evol/DT/step_merged.py

6f265ce

Update posydon/binary_evol/DT/step_merged.py

9b3aa9f

Update posydon/binary_evol/DT/step_merged.py

c0976d8

close parentheses

f013d56

sgossage reviewed Mar 20, 2026

View reviewed changes

commenting out binary.event = None

d019ef4

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!

maxbriel commented Mar 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sgossage Mar 14, 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!

sgossage Mar 14, 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!

sgossage Mar 22, 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!

sgossage Mar 14, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgossage commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxbriel commented Mar 22, 2026

Uh oh!

sgossage commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ezapartas Feb 12, 2026 •

edited

Loading

sgossage Mar 14, 2026 •

edited

Loading

sgossage Mar 14, 2026 •

edited

Loading

sgossage Mar 22, 2026 •

edited

Loading

sgossage Mar 14, 2026 •

edited

Loading

sgossage commented Mar 21, 2026 •

edited

Loading

sgossage commented Mar 22, 2026 •

edited

Loading