Flexible initial stellar type

[reinhold-willcox]

Just the old code for starting from HeMS or any of the compact objects. Does not include stars where tau may be non-zero.

[jeffriley]

Just a question:

If "initial" (especially wrt to stellar type, but also to other attributes) is going to refer to the stellar type the user wants to start (continue? we're assuming were continuing evolution of an already evolved star, right - we don't really think this is when they started evolving...) evolution from, viz.

**--initial-stellar-type** |br|
Initial stellar type for a single star when evolving in SSE mode. |br|
Options: { MS, HeMS, HeWD, COWD, ONeWD, NS, BH } |br|
Corresponding to Main Sequence, Helium Main Sequence, Helium White Dwarf, Carbon-Oxygen White Dwarf, Oxygen-Neon
White Dwarf, Neutron Star, and Black Hole.
Default = MS

shouldn't you rename all the stellar type Initialise() functions to something else (not sure what exactly), and then rename the FastForward() functions Initalise()?

I have no objection to the functionality, but I still think using the term "initial" when you really mean "sometime after birth" is asking for confusion...

[jeffriley]

Check discussions in #1036

[ilyamandel]

Thank you for the PR, @reinhold-willcox !

I have a list of questions/requests below, in addition to @jeffriley 's comments (and often repeating them, but I haven't explicitly checked against Jeff's comments).

It looks to me like there are a few places in the code where things were changed by copy-pasting, but new thinking is required if the initial stellar types are not MS stars. For example, consider the code in BaseBinaryStar.cpp from lines ~176 onward. Here, we are dealing with the case when stars are in RLOF at birth. But does it make sense for, say, two WDs or BHs to be in an "over-contact binary", yielding equal masses? I don't think so. And the comments are also not accurate, as they still refer to ZAMS ("// create new stars with equal masses - all other ZAMS values recalculated" -- see line 191).

We also need to think about outputs carefully. For example,
STAR_1_PROPERTY::MZAMS -- what will this actually log for a star that didn't start on the MS?

A spurious commented out function left in BaseStar.h, also Star.h:
// RTW void IncrementOmega(const double p_OmegaDelta) { m_Omega += p_OmegaDelta; } // Apply delta to current m_Omega

Why are HeMS radius and luminosity inititially computed using functions that are designed for computations at ZAMS (see FastForward() in HeMS.h) :
m_Radius = CalculateRadiusAtZAMS_Static(m_MZAMS);
m_Luminosity = CalculateLuminosityAtZAMS_Static(m_MZAMS);

BTW, most of the other FastForward functions are identical -- do you want to use inheritance instead of redefining them for each stellar type?

Options.cpp, lines 257-263: I don't understand the comments that the stellar types will revert to main sequence later, would they not change to the user-requested initial types (possibly defaulting to MS if not otherwise specified)?

Given new options, whatsnew documentation, yaml.h, and the default YAML file should be updated.

Have you checked that the initial values and subsequent evolution of non-ZAMS stars (e.g., HeMS, WDs, etc.) are sensible? (I see no reason why they shouldn't be, but there's always a risk we forgot to initialise something important, so it's important to check...)

[jeffriley]

In the interests of moving this forward, I withdraw my objections.

[reinhold-willcox]

It looks to me like there are a few places in the code where things were changed by copy-pasting, but new thinking is required if the initial stellar types are not MS stars. For example, consider the code in BaseBinaryStar.cpp from lines ~176 onward. Here, we are dealing with the case when stars are in RLOF at birth. But does it make sense for, say, two WDs or BHs to be in an "over-contact binary", yielding equal masses? I don't think so. And the comments are also not accurate, as they still refer to ZAMS ("// create new stars with equal masses - all other ZAMS values recalculated" -- see line 191).

Fixed by adding extra conditions to the mass-equilization section

We also need to think about outputs carefully. For example, STAR_1_PROPERTY::MZAMS -- what will this actually log for a star that didn't start on the MS?

I have not touched this. Right now it will report whatever was used as the initial mass of the non-ZAMS star. I agree this may be confusing for users, but fixing this requires the careful decoupling of ZAMS / initial variable names.

A spurious commented out function left in BaseStar.h, also Star.h: // RTW void IncrementOmega(const double p_OmegaDelta) { m_Omega += p_OmegaDelta; } // Apply delta to current m_Omega

Fixed.

Why are HeMS radius and luminosity inititially computed using functions that are designed for computations at ZAMS (see FastForward() in HeMS.h) : m_Radius = CalculateRadiusAtZAMS_Static(m_MZAMS); m_Luminosity = CalculateLuminosityAtZAMS_Static(m_MZAMS);

Fixed.

BTW, most of the other FastForward functions are identical -- do you want to use inheritance instead of redefining them for each stellar type?

Agreed, applied for the types that inherent from Remnants.h

Options.cpp, lines 257-263: I don't understand the comments that the stellar types will revert to main sequence later, would they not change to the user-requested initial types (possibly defaulting to MS if not otherwise specified)?

For this purpose, the default initial-stellar-type is STELLAR_TYPE::STAR. If the user does not set this to something else, the star will later determine if it should switch to CHE, or if not, MS > 0.7 or < 0.7. The comment is to clarify this for any users who might think "I just want to initiliase a MS star, why is that not an option". I am open to improved ways of presenting this.

Given new options, whatsnew documentation, yaml.h, and the default YAML file should be updated.

Working on this now.

Have you checked that the initial values and subsequent evolution of non-ZAMS stars (e.g., HeMS, WDs, etc.) are sensible? (I see no reason why they shouldn't be, but there's always a risk we forgot to initialise something important, so it's important to check...)

I have previously used this code for a number of different binary types (most recently, a small population of HeMS+NS binaries), and found that it worked well and gave sensible results. Users should really be aware that this is not the traditional functionality of COMPAS and so, e.g., they should supply all initial masses and separations explicitly without resorting to the defaults. But as you point out, there was previously the possibility of initializing a DWD that would immediately equalize, which I hadn't caught. There may well be other bugs here that haven't been identified yet, but that is always a risk.

[ilyamandel]

Thanks, @reinhold-willcox.

Are there any checks on the initial masses for the advanced types? Can you have a user initialise a 0.01 solar mass ONeWD, for example? Is that sensible? Or a 0.01 solar mass HeMS star? Would Hurley tracks extend to those masses and allow such a star to evolve?

On meaningless ZAMS values such as m_MZAMS, m_LZAMS, etc. -- why not reset them to zero in SetUpStarWhenNotStartingAtZAMS()?

BTW, are the Initialise() functions still called in this case? I.e., would a HeMS star set up in this way have the right values of m_HydrogenAbundanceSurface and m_HeliumAbundanceSurface?

[jeffriley]

We also need to think about outputs carefully. For example, STAR_1_PROPERTY::MZAMS -- what will this actually log for a
star that didn't start on the MS?

I have not touched this. Right now it will report whatever was used as the initial mass of the non-ZAMS star. I agree this may
be confusing for users, but fixing this requires the careful decoupling of ZAMS / initial variable names.

A simple fix (kludge, actually) for this might be to change the BaseStar::StellarPropertyValue() (and BaseBinaryStar equivalent) to return a sentinel value (maybe -1.0?) for MZAMS (and maybe other ZAMS values - RZAMS for example) if the starting ST wasn't MS. That doesn't fix any internal code problems that might arise from using MZAMS, RZAMS, etc., when they are not really ZAMS values, but it at least might help reduce confusion for users looking at the log file values.

[ilyamandel]

Actually, I think it's a good idea to set things like m_MZAMS, etc., to zero explicitly. If that breaks something, it means that we are relying on their values being sensible -- and, in that case, we should think much harder about what those values ought to be if we initialise stars at later times than ZAMS.

[reinhold-willcox]

These are all good points. As for starting a star at a very low mass value (e.g. a 0.01 Msun HeMS star), I think this should be on the user to ensure they are implementing sensible inputs. We also allow for 200 Msun normal MS stars, but we know they are not really reliable in the Hurley tracks.

As for setting the various ZAMS values to 0, I agree that this is the correct long term approach when we have the time to disentangle ZAMS and initial parameters, but doing this now will surely break things and it will take some effort to separate these carefully.

I think you are correct that we do not call Initialise(), so some of the more subtle internal parameters are not being set correctly.

[ilyamandel]

We also allow for 200 Msun normal MS stars

$ ./COMPAS --mode SSE --initial-mass 200
Commandline Options error: Initial mass (--initial-mass) must be between 0.000070 and 150.000000 Msol
Use option '-h' (or '--help') to see (descriptions of) available options

We don't allow things which are likely to cause our code to crash. :)

[ilyamandel]

but doing this now will surely break things and it will take some effort to separate these carefully.

If true, that tells me we are using ZAMS values for something, but in a way which is not understood. I actually don't think we should be, because we should not need to know the ZAMS mass to evolve a helium star or a compact object. But if we do, then I am with @jeffriley on this not being ready to be merged in.

[reinhold-willcox]

That's true, my bad. But 150 is still well outside of the recommended range.

[ilyamandel]

Agreed. We can't promise that anything about our code is right. But we should try to understand what it does -- i.e., make assumptions with intent rather than by accident. And we should make sure that if it fails, it does so reasonably elegantly, with an informative error message...

[ilyamandel]

Hi @reinhold-willcox ,
@jeffriley pointed out that so many things will change once his new one-step-at-a-time formalism is brought in that this partially completed PR will require a new approach, anyway. Therefore, he suggests closing this for now, and returning to this at a later stage. I agree, so am closing this PR for now.