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

Incorrect brief evolutionary outputs printed to screen #1286

Closed
ilyamandel opened this issue Nov 25, 2024 · 8 comments
Closed

Incorrect brief evolutionary outputs printed to screen #1286

ilyamandel opened this issue Nov 25, 2024 · 8 comments
Assignees
Labels
severity_minor This bug is not very severe urgency_low This issue is not urgent

Comments

@ilyamandel
Copy link
Collaborator

The brief outcome of the evolution printed to the screen is sometimes incorrect. For example, the following

./COMPAS -n 1 --initial-mass-1 1 --initial-mass-2 1 --semi-major-axis 0.03 --detailed-output --logfile-type CSV --evolve-main-sequence-mergers

leads to the output

0: Stars merged: (Main_Sequence_>0.7 -> Helium_White_Dwarf) + (Main_Sequence>_0.7 -> Helium_White_Dwarf)

In fact, the stars merge earlier, as giants, not as HeWDs.

@ilyamandel ilyamandel added severity_minor This bug is not very severe urgency_low This issue is not urgent labels Nov 25, 2024
@jeffriley
Copy link
Collaborator

jeffriley commented Nov 25, 2024

@ilyamandel the output is correct - it's only reporting what happened during the evolution of the binary.

The problem, I think, is that we are not flagging the merger early enough in BaseBinaryStar::ResolveCommonEnvelopeEvent()

We set the stellar merger flag here:

if (utils::Compare(m_SemiMajorAxis, 0.0) <= 0 || utils::Compare(m_Star1->Radius() + m_Star2->Radius(), m_SemiMajorAxis * AU_TO_RSOL) > 0) {
    m_Flags.stellarMerger = true;
}

but that is after we've allowed both stars to call ResolveEnvelopeLossAndSwitch(). I think we should set the stellar merger flag earlier, here:

if (isDonorMS || (!envelopeFlag1 && !envelopeFlag2)) {                                                              // stellar merger
    m_MassTransferTrackerHistory = MT_TRACKING::MERGER; 
    m_Flags.stellarMerger        = true;
}
else if ((m_Star1->DetermineEnvelopeType()==ENVELOPE::RADIATIVE && !m_Star1->IsOneOf(ALL_MAIN_SEQUENCE)) ||
         (m_Star2->DetermineEnvelopeType()==ENVELOPE::RADIATIVE && !m_Star2->IsOneOf(ALL_MAIN_SEQUENCE)) ) {        // check if we have a non-MS radiative-envelope star
    if (!OPTIONS->AllowRadiativeEnvelopeStarToSurviveCommonEnvelope() && OPTIONS->CommonEnvelopeFormalism()!=CE_FORMALISM::TWO_STAGE) { // stellar merger
        m_CEDetails.optimisticCE     = true;
        m_MassTransferTrackerHistory = MT_TRACKING::MERGER;
        m_Flags.stellarMerger        = true;
    }
}

but we don't... (is the isDonorMS flag being set correctly? that's not it - as you said, they were giants (FGB))

@jeffriley
Copy link
Collaborator

Ok... FGB stars have CONVECTIVE envelopes... so we don't consider ot a stellar merger at that point - hence both stars continue to evolve (and switch stellar type), then we set the stellar merger flag. So the stars started as MS stars, and ended evolution as HeWD stars, with the merger happening (according to where we set the flag) after they both switched to HeWD. Is the earlier check for stellar merger correct?

@ilyamandel
Copy link
Collaborator Author

Physically, the stars merge while they are FGB stars.

@jeffriley
Copy link
Collaborator

jeffriley commented Nov 25, 2024

I get that, but that's not what's happening in the code... So, the output is correct, but COMPAS evolution is not. If we want to report what should be happening, we need to set the stellar merger flag earlier and not let the stars evolve to HeWD stars.

EDIT: ...the output is correct (wrt to the code)

@ilyamandel
Copy link
Collaborator Author

The output is always correct with respect to the code: the computer can only do what we tell it to do. ;-)

What I meant was that users may care whether stars merge as FGB stars or as WDs (e.g., the mergers would look very different observationally), so we should fix how we report these mergers.

@jeffriley
Copy link
Collaborator

jeffriley commented Nov 26, 2024

I agree - but my point is that the code as it stands allows the stars to continue evolving after (what we are saying should be) the stellar merger (so it's not that we are reporting the result incorrectly, we are calculating the result incorrectly) - because we don't recognise that the merger has happened at the right place in the code. I think the condition noted above for setting the merger flag is either wrong or deficient - but I don't know what it should be :-)

EDIT: you'll see in the code that if the merger flag is set correctly at the condition noted, the code that evolves the stars forward to HeWD stars is not executed, so the status printed will be correct (the stars will be shown as FGB stars)

@ilyamandel
Copy link
Collaborator Author

@jeffriley -- not as easy as I expected: I seemingly fixed this in #1291, and for the example in the original issue report I now see that m_Flags.stellarMerger is true and both types are still FGB at the end of BaseBinaryStar::ResolveCommonEnvelopeEvent().
Yet they are still being reported as WD mergers, so we must be updating the stellar types somewhere later, despite the merger...
To be fixed. :)

@ilyamandel
Copy link
Collaborator Author

After #1291 , these are being recorded as massless remnants in the brief screen output, but are correctly labeled as a CE between two FGB stars. Maybe there's a better way to present those brief screen outputs, but happy with the existing solution for now, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity_minor This bug is not very severe urgency_low This issue is not urgent
Projects
None yet
Development

No branches or pull requests

2 participants