Initializing a star not at ZAMS

[reinhold-willcox]

Added option and code functionality for the user to set the stellar type they want at Zams. This is useful for anyone working with, e.g, He stars, X-ray binaries, WDs etc. who want to have more control over the initial conditions of their post-MS binary. Due to the non-trivial value of the tau age parameter, we should only allow users to initialize MS, HeMS, WD (any), NS, or BH stars.

This is the same code as in PR #931, but it was easier to create a new branch than to do the merge with the old one.

[reinhold-willcox]

@ilyamandel

1. Why are several properties (e.g., STAR___PROPERTY::MZAMS, STAR___PROPERTY::MASS_0, STAR_*_PROPERTY::INITIAL_STELLAR_TYPE) removed from the standard logfile definition?

Fixed

2. Can we rename FastForward() into something clearer, like "SetInitialStellarParameters" (@jeffriley may have a better suggestion, but FastForward is very ambiguous)?

Deferring to @jeffriley

3. Why does NS require both m_MomentOfInertia (in CGS, according to comments) and m_MomentOfInertia_CGS at initialisation?

Fixed

4. HeMS -- why are the initial radius and luminosity based on the ZAMS mass? [e.g., m_Radius = CalculateRadiusAtZAMS_Static(m_MZAMS); ]

Within the FastForward function, these are the initially sampled values. As in, if the user specifies an initial mass and that they want to start with a HeMS star, they presumably want that mass for the He star (and not the mass of the HeZams star that would form from a standard ZAMS star of that mass). I am happy to change this function argument to something more obvious / sensible, but I'll defer to @jeffriley on the best choice.

5. changelog.h -- new comment seems out of order

Fixed

6. Not sure I follow the change from plural to singular in the comments of main.cpp (e.g., line 236)

I don't remember why this change occurred. Happy to change back, except that main.cpp is supposed to refer to just the single stars, i.e we're not yet "thinking" about binaries, so possibly the change was made to reflect that. Deferring to @jeffriley who has thought more about this than I have.

7. constants.h: Not sure about "Allowed initial stellar types (STAR is used here as the default MS in place of either MS_LTE_07 or MS_GT_07 as these get set later depending on initial mass"; if we need a super-type label specifically for MS stars, should we define that explicitly?

Deferring to @jeffriley

8. constants.h -- why move BINARY_PROPERTY::UNBOUND to now appear between STAR_2_PROPERTY::MASS_0 and STAR_1_PROPERTY::MASS?

Fixed

9. program-options-list-defaults.rst -- add the new program options to the list at the bottom of the file

Fixed

10. Since there are new program options, re-generate the default yaml file

Ahh, I don't remember how to do this! Isn't it supposed to happen automatically?

11. Are these changes significant enough to warrant a mention in the major changes list?

Deferring to @jeffriley. I'm happy either way.

[ilyamandel]

@jeffriley -- there were some queries for you above; perhaps you could add your review?

[jeffriley]

I will get to this soon - it's 1 or 2 on my list of reviews. I just need to refresh my memory - when Reinhold and I first got this working I think I was happy with the functionality - but not the nomenclature. I'll need to go over that. I think I vaguely remember thinking we should introduce an option for users to specify tau for the stellar types for which we can't calculate tau (so can't start evolution there). Anyway, I'll get to it - promise.

[jeffriley]

@reinhold-willcox @ilyamandel @SimonStevenson @avigna

This will be off the top of my head, and somewhat brief (as brief as I can make it...).

I haven't reviewed the code in detail - I can see that there are some minor issues that can be remedied easily, but my biggest issue with the PR isn't the coding, it is, as it always has been, on the surface, nomenclature - but that really just points to underlying issues. When @reinhold-willcox first raised the question of whether it was possible to do this, I thought the code changes would be fairly trivial, and we discussed it over Zoom and came up with a few hacks to the code that made it work. I still think the actual code changes are fairly trivial, but that we are missing something more important. We discussed this in some COMPAS dev meetings a long time ago, but I don't think we ever resolved it.

The PR title, and name of the branch: "Generalized zams stellar type" give it away. That's not what it is... COMPAS, without this PR, evolves zero-age main sequence stars to some conclusion. This PR changes that to evolving stars from some arbitrary stellar type to some conclusion, and the arbitrary stellar type is not "generalized ZAMS".

Here are just a few of the issues. This is not an exhaustive list, but I think gives a taste of the things that we need to think about that we haven't... and none of these issue are insurmountable - we just need to think more carefully about how to do this:

• by calling the new options initial-stellar-type (and variants initial-stellar-type-1 and initial-stellar-type-2), we are at best overloading the meaning of "initial", and at worst changing the meaning entirely.

Currently, in my opinion, "initial" has a special meaning in COMPAS - (to me) it means "at birth", or "zero-age main sequence" (I know, or at least think I know, that "birth" and "ZAMS" are technically not the same thing, but in COMPAS they virtually are the same thing.

If we are going to change the meaning of "initial" in COMPAS, at the very least we need to document very clearly what we (now - after this PR) mean by "initial". How does this new meaning of "initial" affect or relate to things like the "initial" mass functions we use? Am I wrong in thinking that users understand that the IMF describes the distribution of masses during star formation (i.e. at birth), not for some arbitrary time after birth at which we've decided to start our evolutionary simulation? We need to check everywhere in the code we've used the word "initial" - both in variable names, and in documentation/comments - to ensure we don't leave confusing/ambiguous code/comments there. We probably should have a section in the documentation to explain what we now mean evolving stars from some arbitrary point to mean, and how it works, especially vis-a-vis how it currently works (i.e. from ZAMS).

• "initial-mass", "initial-mass-1", and "initial-mass-2"

We have options to specify the initial mass of a star, or stars - currently they are understood to mean the mass of the star(s) at birth (or ZAMS), but that meaning has been overloaded with this PR. The "initial" mass in COMPAS is tied to the IMF and IMF options - even after this PR. Running COMPAS as with the command:

./compas -n 1 --initial-stellar-type-1 HeWD --initial-stellar-type-2 COWD

evolves a binary with constituents assigned stellar types HeWD and COWD, with masses drawn from the default IMF. Is that reasonable? I don't know... Should users expect that? Again, I don't know. Should the mass ranges we accept for these options change depending upon the initial stellar type we set? I don't know... If we're going to go that way, we should document it clearly. There are almost certainly other options that we should think about similarly.

Maybe a solution is to change the initial-mass* options to just mass* - i.e. remove initial-mass, initial-mass-1, and initial-mass-2, and just have mass, mass-1, and mass-2. Then we can document those options as being the mass assigned to the star(s) at the start of evolution (which may be ZAMS, or it may be at any arbitrarily assigned stellar type). We may have to look at other options to see if they need to be renamed (and documented), and/or how they interact with the new meaning of "initial".

• output file headers for initial* values - currently we have headers like Mass@ZAMS, SemiMajorAxis@ZAMS, etc. We also have StellarType@ZAMS, which currently can only be Main_Sequence_<=_0.7 or Main_Sequence_>_0.7, but could now (after this PR) be something else (which doesn't seem to make a lot of sense to me). The other *@ZAMS headers will not be right with this PR - the mass(es) we assign star(s) at the start of evolution are not @ZAMS. That's probably an easy fix, but again, it's a paradigm change that needs to be documented clearly.

• variable names, in-code documentation and comment

We need to ensure that these are all consistent with the new paradigm...

• I think if we do this we add an option to allow users to specify tau so that users can start evolution at all stellar types

This would have to be implemented carefully, but I think it is possible. If a user has some previous runs for which they have detailed output files, it's feasible that they could start evolution at a particular stellar type and supply tau from one of the previous runs, then tweak one or two other options, or change the companion, etc., just to see what happens. I think with a bit of thought we can provide that functionality.

=========================
Summary:

I think at a minimum we risk confusing our users, and worst case we risk confusing ourselves when we write the code to support what I think it will end up being a confused and confusing implementation (that's not meant to indicate that the coding here is in any way deficient, or not up to standard - just that we haven't really thought this through (what I would call) properly). That doesn't mean I don't think we should do this - I think it is useful functionality, and I think we should implement it but we need to do it in a more logical and less confusing way. I think that requires a lot more thought and time than we have put into it to get it to this stage, and I think if we do it properly, it will change COMPAS functionality enough to justify changing the version to v03.00.00 (i.e this is a major change, not a minor change/enhancement).

I know time is an issue for most of us - I'm willing to do this, with help from others if and when they have time. I don't think we should implement this PR as is with a view to working towards a better implementation over time - a few of us might know what's going on (i.e. when we use --initial-stellar-type*, headers with ZAMS in them mightn't actually mean ZAMS, etc.), but the majority of the user base, and especially newer users would, in my opinion, struggle to make sense of the ambiguity ("Caveat Utilitor!" is not reasonable in my opinion). I think that would be a backwards step for COMPAS, especially since we've all worked hard to make the COMPAS codebase easy to understand (if not intuitive) and reasonably easy to maintain/extend.

I am well aware that I see things from a less-experienced perspective, so I am happy to be told I'm wrong about this being a paradigm change rather than just a minor enhancement, and that most users would take it in their stride...

[jeffriley]

10. Since there are new program options, re-generate the default yaml file
    Ahh, I don't remember how to do this! Isn't it supposed to happen automatically?

No - otherwise we'd have a new yaml file created every time someone ran COMPAS. Use --create-yaml-file, and, optionally,
--yaml-template if you want to specify your own yaml file template.

[jeffriley]

6. Not sure I follow the change from plural to singular in the comments of main.cpp (e.g., line 236)
   I don't remember why this change occurred. Happy to change back, except that main.cpp is supposed to refer to just the single
   stars, i.e we're not yet "thinking" about binaries, so possibly the change was made to reflect that. Deferring to @jeffriley who has
   thought more about this than I have.

Line 236 in main.cpp is in the EvolveSingleStars() function (used in SSE mode), so we should not be talking about binaries and companions there. EvolveSingleStars() is where we initialise the mass for a single star.

There is an equivalent EvolveBinaryStars() function in main.cpp (used in BSE mode) - but the constituent star masses are initialised in the BaseBinaryStar constructor.

[jeffriley]

7. constants.h: Not sure about "Allowed initial stellar types (STAR is used here as the default MS in place of either MS_LTE_07
   or MS_GT_07 as these get set later depending on initial mass"; if we need a super-type label specifically for MS stars, should
   we define that explicitly?
   Deferring to @jeffriley

Yes.

[jeffriley]

4. HeMS -- why are the initial radius and luminosity based on the ZAMS mass? [e.g., m_Radius =
   CalculateRadiusAtZAMS_Static(m_MZAMS); ]
   Within the FastForward function, these are the initially sampled values. As in, if the user specifies an initial mass and that they
   want to start with a HeMS star, they presumably want that mass for the He star (and not the mass of the HeZams star that
   would form from a standard ZAMS star of that mass). I am happy to change this function argument to something more
   obvious / sensible, but I'll defer to @jeffriley on the best choice.

I think this exemplifies my main objection.

Within the FastForward function, these are the initially sampled values. As in, if the user specifies an initial mass...

If they are initially sampled, the user didn't specify them; if the user specified them, they weren't initially sampled.
If the user didn't specify (e.g.) the initial mass, then m_MZAMS will have been sampled - from the IMF (that's one of my
objections, noted above).

I am happy to change this function argument to something more obvious / sensible...

Argument or name? The function doesn't have an argument - do you mean the argument to the ZAMS functions called within the function, or do you actually mean the name of the function (`FastForward')?

EDIT: Ah, I think you mean argument... This applies to the name:

2. Can we rename FastForward() into something clearer, like "SetInitialStellarParameters" (@jeffriley may have a better
   suggestion, but FastForward is very ambiguous)?

So are you referring (above) to changing the arguments to the functions CalculateRadiusAtZAMS_Static(), CalculateLuminosityAtZAMS_Static(), and CalculateTemperatureOnPhase_Static()? (I'm specifically looking at HeMS::FastForward().

[ilyamandel]

Briefly on my phone: Jeff raises some very good points. I agree we should be careful about nomenclature. I am fine with us using initial to refer to properties at the start of the orbital calculation. That includes initial conditions and their distributions, including applying them to non-ZAMS stars. I am not fine with overloading ZAMS, including in outputs (or changing output names, in case anyone suggests that -- adding new outputs is fine, but ***@***.*** should be something like -1 if we don't actually start at ZAMS). ZAMS means something very specific -- zero age main sequence -- and should keep that meaning.

…

On Tue, 27 Feb 2024, 7:49 pm jeffriley, ***@***.***> wrote: @reinhold-willcox <https://github.com/reinhold-willcox> @ilyamandel <https://github.com/ilyamandel> @SimonStevenson <https://github.com/SimonStevenson> @avigna <https://github.com/avigna> This will be off the top of my head, and somewhat brief (as brief as I can make it...). I haven't reviewed the code in detail - I can see that there are some minor issues that can be remedied easily, but my biggest issue with the PR isn't the coding, it is, as it always has been, on the surface, nomenclature - but that really just points to underlying issues. When @reinhold-willcox <https://github.com/reinhold-willcox> first raised the question of whether it was possible to do this, I thought the code changes would be fairly trivial, and we discussed it over Zoom and came up with a few hacks to the code that made it work. I still think the actual code changes are fairly trivial, but that we are missing something more important. We discussed this in some COMPAS dev meetings a long time ago, but I don't think we ever resolved it. The PR title, and name of the branch: "Generalized zams stellar type" give it away. That's not what it is... COMPAS, without this PR, evolves zero-age main sequence stars to some conclusion. This PR changes that to evolving stars from some arbitrary stellar type to some conclusion, and the arbitrary stellar type is not "generalized ZAMS". Here are just a few of the issues. This is not an exhaustive list, but I think gives a taste of the things that we need to think about that we haven't... and none of these issue are insurmountable - we just need to think more carefully about how to do this: - by calling the new options initial-stellar-type (and variants initial-stellar-type-1 and initial-stellar-type-2), we are at best overloading the meaning of "initial", and at worst changing the meaning entirely. Currently, in my opinion, "initial" has a special meaning in COMPAS - (to me) it means "at birth", or "zero-age main sequence" (I know, or at least think I know, that "birth" and "ZAMS" are technically not the same thing, but in COMPAS they virtually are the same thing. If we are going to change the meaning of "initial" in COMPAS, at the very least we need to document very clearly what we (now - after this PR) mean by "initial". How does this new meaning of "initial" affect or relate to things like the "initial" mass functions we use? Am I wrong in thinking that users understand that the IMF describes the distribution of masses during star formation (i.e. at birth), not for some arbitrary time after birth at which we've decided to start our evolutionary simulation? We need to check everywhere in the code we've used the word "initial" - both in variable names, and in documentation/comments - to ensure we don't leave confusing/ambiguous code/comments there. We probably should have a section in the documentation to explain what we now mean evolving stars from some arbitrary point to mean, and how it works, especially vis-a-vis how it currently works (i.e. from ZAMS). - "initial-mass", "initial-mass-1", and "initial-mass-2" We have options to specify the initial mass of a star, or stars - currently they are understood to mean the mass of the star(s) at birth (or ZAMS), but that meaning has been overloaded with this PR. The "initial" mass in COMPAS is tied to the IMF and IMF options - even after this PR. Running COMPAS as with the command: ./compas -n 1 --initial-stellar-type-1 HeWD --initial-stellar-type-2 COWD evolves a binary with constituents assigned stellar types HeWD and COWD, with masses drawn from the default IMF. Is that reasonable? I don't know... Should users expect that? Again, I don't know. Should the mass ranges we accept for these options change depending upon the initial stellar type we set? I don't know... If we're going to go that way, we should document it clearly. There are almost certainly other options that we should think about similarly. Maybe a solution is to change the initial-mass* options to just mass* - i.e. remove 'initial-mass, initial-mass-1, and initial-mass-2, and just have mass, mass-1, and mass-2`. Then we can document those options as being the mass assigned to the star(s) at the start of evolution (which may be ZAMS, or it may be at any arbitrarily assigned stellar type). We may have to look at other options to see if they need to be renamed (and documented), and/or how they interact with the new meaning of "initial". - output file headers for initial* values - currently we have headers like ***@***.***, ***@***.***, etc. We also have ***@***.***, which currently can only be Main_Sequence_<=_0.7 or Main_Sequence_>_0.7, but could now (after this PR) be something else (which doesn't seem to make a lot of sense to me). The other ***@***.*** headers will not be right with this PR - the mass(es) we assign star(s) at the start of evolution are not @zams. That's probably an easy fix, but again, it's a paradigm change that needs to be documented clearly. - variable names, in-code documentation and comment We need to ensure that these are all consistent with the new paradigm... - I think if we do this we add an option to allow users to specify tau so that users can start evolution at all stellar types This would have to be implemented carefully, but I think it is possible. If a user has some previous runs for which they have detailed output files, it's feasible that they could start evolution at a particular stellar type and supply tau from one of the previous runs, then tweak one or two other options, or change the companion, etc., just to see what happens. I think with a bit of thought we can provide that functionality. ========================= Summary: I think at a minimum we risk confusing our users, and worst case we risk confusing ourselves when we write the code to support what I think it will end up being a confused and confusing implementation (that's not meant to indicate that the coding here is in any way deficient, or not up to standard - just that we haven't really thought this through (what I would call) properly). That doesn't mean I don't think we should do this - I think it is useful functionality, and I think we should implement it but we need to do it in a more logical and less confusing way. I think that requires a lot more thought and time than we have put into it to get it to this stage, and I think if we do it properly, it will change COMPAS functionality enough to justify changing the version to v03.00.00 (i.e this is a major change, not a minor change/enhancement). I know time is an issue for most of us - I'm willing to do this, with help from others if and when they have time. I don't think we should implement this PR as is with a view to working towards a better implementation over time - a few of us might know what's going on (i.e. when we use --initial-stellar-type*, headers with ZAMS in them mightn't actually mean ZAMS, etc.), but the majority of the user base, and especially newer users would, in my opinion, struggle to make sense of the ambiguity ("Caveat Utilitor!" is not reasonable in my opinion). I think that would be a backwards step for COMPAS, especially since we've all worked hard to make the COMPAS codebase easy to understand (if not intuitive) and reasonably easy to maintain/extend. I am well aware that I see things from a less-experienced perspective, so I am happy to be told I'm wrong about this being a paradigm change rather than just a minor enhancement, and that most users would take it in their stride... — Reply to this email directly, view it on GitHub <#1036 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD5BVKPSHJX3NYCKF25BSDYVWMZJAVCNFSM6AAAAABA2FGVKOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWGA2TONJUHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ilyamandel]

Mass AT ZAMS is what I meant to say when github's mail parser replaced it with ***... :-)

…

On Wed, 28 Feb 2024, 6:16 pm Ilya Mandel, ***@***.***> wrote: Briefly on my phone: Jeff raises some very good points. I agree we should be careful about nomenclature. I am fine with us using initial to refer to properties at the start of the orbital calculation. That includes initial conditions and their distributions, including applying them to non-ZAMS stars. I am not fine with overloading ZAMS, including in outputs (or changing output names, in case anyone suggests that -- adding new outputs is fine, but ***@***.*** should be something like -1 if we don't actually start at ZAMS). ZAMS means something very specific -- zero age main sequence -- and should keep that meaning. On Tue, 27 Feb 2024, 7:49 pm jeffriley, ***@***.***> wrote: > @reinhold-willcox <https://github.com/reinhold-willcox> @ilyamandel > <https://github.com/ilyamandel> @SimonStevenson > <https://github.com/SimonStevenson> @avigna <https://github.com/avigna> > > This will be off the top of my head, and somewhat brief (as brief as I > can make it...). > > I haven't reviewed the code in detail - I can see that there are some > minor issues that can be remedied easily, but my biggest issue with the PR > isn't the coding, it is, as it always has been, on the surface, > nomenclature - but that really just points to underlying issues. When > @reinhold-willcox <https://github.com/reinhold-willcox> first raised the > question of whether it was possible to do this, I thought the code changes > would be fairly trivial, and we discussed it over Zoom and came up with a > few hacks to the code that made it work. I still think the actual code > changes are fairly trivial, but that we are missing something more > important. We discussed this in some COMPAS dev meetings a long time ago, > but I don't think we ever resolved it. > > The PR title, and name of the branch: "Generalized zams stellar type" > give it away. That's not what it is... COMPAS, without this PR, evolves > zero-age main sequence stars to some conclusion. This PR changes that to > evolving stars from some arbitrary stellar type to some conclusion, and the > arbitrary stellar type is not "generalized ZAMS". > > Here are just a few of the issues. This is not an exhaustive list, but I > think gives a taste of the things that we need to think about that we > haven't... and none of these issue are insurmountable - we just need to > think more carefully about how to do this: > > - by calling the new options initial-stellar-type (and variants > initial-stellar-type-1 and initial-stellar-type-2), we are at best > overloading the meaning of "initial", and at worst changing the meaning > entirely. > > Currently, in my opinion, "initial" has a special meaning in COMPAS - (to > me) it means "at birth", or "zero-age main sequence" (I know, or at least > think I know, that "birth" and "ZAMS" are technically not the same thing, > but in COMPAS they virtually are the same thing. > > If we are going to change the meaning of "initial" in COMPAS, at the very > least we need to document very clearly what we (now - after this PR) mean > by "initial". How does this new meaning of "initial" affect or relate to > things like the "initial" mass functions we use? Am I wrong in thinking > that users understand that the IMF describes the distribution of masses > during star formation (i.e. at birth), not for some arbitrary time after > birth at which we've decided to start our evolutionary simulation? We need > to check everywhere in the code we've used the word "initial" - both in > variable names, and in documentation/comments - to ensure we don't leave > confusing/ambiguous code/comments there. We probably should have a section > in the documentation to explain what we now mean evolving stars from some > arbitrary point to mean, and how it works, especially vis-a-vis how it > currently works (i.e. from ZAMS). > > - "initial-mass", "initial-mass-1", and "initial-mass-2" > > We have options to specify the initial mass of a star, or stars - > currently they are understood to mean the mass of the star(s) at birth (or > ZAMS), but that meaning has been overloaded with this PR. The "initial" > mass in COMPAS is tied to the IMF and IMF options - even after this PR. > Running COMPAS as with the command: > > ./compas -n 1 --initial-stellar-type-1 HeWD --initial-stellar-type-2 COWD > > evolves a binary with constituents assigned stellar types HeWD and COWD, > with masses drawn from the default IMF. Is that reasonable? I don't know... > Should users expect that? Again, I don't know. Should the mass ranges we > accept for these options change depending upon the initial stellar type we > set? I don't know... If we're going to go that way, we should document it > clearly. There are almost certainly other options that we should think > about similarly. > > Maybe a solution is to change the initial-mass* options to just mass* - > i.e. remove 'initial-mass, initial-mass-1, and initial-mass-2, and just > have mass, mass-1, and mass-2`. Then we can document those options as > being the mass assigned to the star(s) at the start of evolution (which may > be ZAMS, or it may be at any arbitrarily assigned stellar type). We may > have to look at other options to see if they need to be renamed (and > documented), and/or how they interact with the new meaning of "initial". > > - > > output file headers for initial* values - currently we have headers > like ***@***.***, ***@***.***, etc. We also have ***@***.***, > which currently can only be Main_Sequence_<=_0.7 or > Main_Sequence_>_0.7, but could now (after this PR) be something else > (which doesn't seem to make a lot of sense to me). The other ***@***.*** > headers will not be right with this PR - the mass(es) we assign star(s) at > the start of evolution are not @zams. That's probably an easy fix, > but again, it's a paradigm change that needs to be documented clearly. > - > > variable names, in-code documentation and comment > > We need to ensure that these are all consistent with the new paradigm... > > - I think if we do this we add an option to allow users to specify tau > so that users can start evolution at all stellar types > > This would have to be implemented carefully, but I think it is possible. > If a user has some previous runs for which they have detailed output files, > it's feasible that they could start evolution at a particular stellar type > and supply tau from one of the previous runs, then tweak one or two > other options, or change the companion, etc., just to see what happens. I > think with a bit of thought we can provide that functionality. > > ========================= > Summary: > > I think at a minimum we risk confusing our users, and worst case we risk > confusing ourselves when we write the code to support what I think it will > end up being a confused and confusing implementation (that's not meant to > indicate that the coding here is in any way deficient, or not up to > standard - just that we haven't really thought this through (what I would > call) properly). That doesn't mean I don't think we should do this - I > think it is useful functionality, and I think we should implement it but we > need to do it in a more logical and less confusing way. I think that > requires a lot more thought and time than we have put into it to get it to > this stage, and I think if we do it properly, it will change COMPAS > functionality enough to justify changing the version to v03.00.00 (i.e this > is a major change, not a minor change/enhancement). > > I know time is an issue for most of us - I'm willing to do this, with > help from others if and when they have time. I don't think we should > implement this PR as is with a view to working towards a better > implementation over time - a few of us might know what's going on (i.e. > when we use --initial-stellar-type*, headers with ZAMS in them mightn't > actually mean ZAMS, etc.), but the majority of the user base, and > especially newer users would, in my opinion, struggle to make sense of the > ambiguity ("Caveat Utilitor!" is not reasonable in my opinion). I think > that would be a backwards step for COMPAS, especially since we've all > worked hard to make the COMPAS codebase easy to understand (if not > intuitive) and reasonably easy to maintain/extend. > > I am well aware that I see things from a less-experienced perspective, so > I am happy to be told I'm wrong about this being a paradigm change rather > than just a minor enhancement, and that most users would take it in their > stride... > > — > Reply to this email directly, view it on GitHub > <#1036 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAD5BVKPSHJX3NYCKF25BSDYVWMZJAVCNFSM6AAAAABA2FGVKOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWGA2TONJUHE> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[avigna]

I think @jeffriley raises good points, and overall agree with his take. I have only gone through this thread and not the PR itself, but here my summarized thoughts:

1. the name of this PR is very problematic, a contradiction in itself. The stellar type at the zero-age main sequence (ZAMS) is the main sequence (MS), and therefore changing the initial stellar type to an arbitrary is non-intuitive. Maybe one could use this for He-ZAMS type, and even then it would be problematic. This is not only a problem in nomenclature, but also in the philosophy of what COMPAS currently does.
2. nomenclature aside, arbitrarily changing the initial stellar type can lead to unintended non-physical behaviour. Assuming a MS star at ZAMS has a BH companion is difficult to reconcile with the isolated binary scenario for COMPAS. In that particular case, it could lead to faulty predictions on lifetimes of, e.g., X-ray binary phases (and overall lifetime in general). We should really make sure setting arbitrary tau's work.
3. the reason why you can do these sort of games in codes like MESA is because you have all of the information about a particular star in your output, and then you can just choose when to pair it with a binary. In COMPAS, we only a have a small fraction of the information. Therefore, again, we would need to make sure we can create an arbitrary star (with a defined -fractional- age and stellar type) with the SSE part of COMPAS. I would propose that as a separate PR for SSE. If this has been done and thoroughly tested in this PR, great (and sorry to bring it up).
4. another alternative is to evolve COMPAS single stars to a certain age and then convert that into a binary. This could certainly work, and clearly defines a single starting point (even when there likely are degeneracies in which initial stars lead to a future stage, but that is besides the point of this PR). This would also neglect effects of binary evolution, but I think is not a bad first step.

Overall, I think a naive approach could be very problematic both code and science wise. If all of the aformenetioned concerns are addressed in this PR, and the implementation is steady and tested, congratulations and sorry for bringing this up. If not, my suggestion would be to make sure this works in SSE before blindly setting things in BSE.

[reinhold-willcox]

@jeffriley @ilyamandel @avigna

I'm hearing from you all that the nomenclature is really the biggest issue at stake here. I'm personally indifferent to what we use, I would just like to get the functionality incorporated into COMPAS, so I am very open to suggestions for alternative nomenclature.

As far as allowing for non-zero tau values, we had previously discussed this and decided it would be a useful extension in the future, but out of scope for this PR. It's worth noting that binary_c allows for exactly this functionality: the creation of non-ZAMS stars in a binary with the requirement that tau=0. @avigna to your point, I don't think it matters if stellar evolution could not create a ZAMS star and a BH together, you can simply choose a wide enough orbit that the interaction happens when the star is at the desired evolutionary phase.

My belief is that if the user is studying these types of systems (meaning, anything that is not ZAMS+ZAMS at birth), it is their responsibility to set e.g the mass distribution, mass-ratio distribution, and period distribution explicitly. We could maybe put a warning into the splash screen to mention this (e.g. "Hey, you are initializing non-ZAMS stars, make sure you specify the mass and period distributions"), but we shouldn't feel obliged to provide these distributions for the users, particularly when they are not well known anyway.

Once the nomenclature has been addressed, I can go in and fix the yaml file. @jeffriley you mentioned wanting to create a new STELLAR_TYPE for "MainSequence" without a mass specification. This seems to me to be a bit of overhead, and outside the scope of what I can do. If you still feel it is important to add, could you make the relevant changes for this?

[avigna]

Hi @reinhold-willcox, thanks for your reply! I more or less agree.

For reference, I don't consider that binary_c doing anything validates a choice, which is nothing against binary_c, more about being conscious about the choices we make.

However... I would be VERY happy with a warning message that says something like what you suggest or that the stars are not coeval or something among those lines. A general message per population might suffice, but I am uncertain if we would have the possibility on a population of evolving some binaries coeval and some not. But yes, please put a printed warning statement... that gets you "off the hook" :)

[jeffriley]

It's worth noting that binary_c allows for exactly this functionality: the creation of non-ZAMS stars in a binary with the
requirement that tau=0

Hurley's SSE allows for the creation of non-ZAMS stars (but requires the mass be entered - so no sampling from IMF). I assume BSE does too, but I haven't looked. I agree with @avigna in that the functionality existing in other tools isn't a good enough reason to implement it in COMPAS - but I don't think that's the issue anyway - I think we all agree the functionality would be useful. My focus is the implementation, and making it consistent with existing COMPAS code and concepts. COMPAS is OO c++ code, so implementing this new functionality in COMPAS requires that we think about how to do it best, consistent with the existing OO implementation. SSE and BSE are not OO code - I haven't looked at binary_c, so I don't know. My point is, it's not the usefulness of the functionality that's in question, it's how well we implement it in COMPAS.

Some comments:

My belief is that if the user is studying these types of systems (meaning, anything that is not ZAMS+ZAMS at birth),
it is their responsibility to set e.g the mass distribution, mass-ratio distribution, and period distribution explicitly.

Indeed, so I think we should not allow the mass for non-ZAMS stars to be sampled from the IMF (I suspect this is why SSE requires the mass be provided by the user).

We could maybe put a warning into the splash screen to mention this
(e.g. "Hey, you are initializing non-ZAMS stars, make sure you specify the mass and period distributions")

If the user is using a grid file, we won't know at the time the splash screen is written that they are initialising non-ZAMS stars. The warning would have to come later, and I think on a per-star/binary basis.

@jeffriley you mentioned wanting to create a new STELLAR_TYPE for "MainSequence" without a mass specification.
This seems to me to be a bit of overhead, and outside the scope of what I can do. If you still feel it is important to add,
could you make the relevant changes for this?

Actually I think that was @ilyamandel, but I agree. The coding is trivial - making it fit with the existing stellar-type hierarchy in a sensible way (and that doesn't interfere with evolution/checks later) will need some thought. I can take a look at that.

Both @ilyamandel and I commented above that we should not output values in columns labelled "*ZAMS" that are not, in fact, ZAMS values. So, for example, the "initial" mass of a star that starts evolution as a non-ZAMS star should not be output the the "Mass@ZAMS" column in any of the log files. I (and I think @ilyamandel also expressed this opinion) don't think we should address that by changing the header to (something like) "Initial_Mass". I think we should create a new column (for mass, and others for other attributes) for the "initial" mass of a star that is created as a non-ZAMS star. That means we should also create new variables in COMPAS to accommodate that.

Quick note - we don't name initial values for semi-major axis and eccentricity as (e.g.) "m_ZAMSeccentricity" - instead we use "m_EccentricityInitial", but we do use "Eccentricity@ZAMS" as the column header. If we're going to use "initial" to mean the value give to a non-ZAMS star at initialisation, we need to reconcile those sorts of anomalies.

I did a cursory search for "ZAMS" in the COMPAS code (so we really should do a more detailed search). Here are some ad hoc comments - no particular oder - based on what I saw (I don't know if any of these have already beed addressed in the code changes for this PR - some of them may have been, but I suspect not all of them):

• MainSequence evolution requires ZAMS values - so need they to be set for MS stars (fits with the comments above re retaining the *ZAMS columns and variables)

• I think the Star and BaseStar constructors assume ZAMS initialisation. I image that has been addressed by the code changes for this PR, but need to check.

• I don't think there's anything specific we need to do for CH stars - they should fall out ok if we initailise MS stars.

• The following functions use ZAMS values, and refer to ZAMS values in their descriptions. if we don't initialise ZAMS stars, the ZAMS values aren't available - and the descriptions will be misleading. Has that been addressed?

  • BaseStar::CalculateLogBindingEnergyLoveridge()
  • BaseStar::CalculateMassLossRateRSGBeasor2020()
  • BaseStar::CalculateMassLossRateRSGDecin2023()
  • BaseStar::CalculateMassLossRateFlexible2023()

• There are a number of functions for which the parameter names are "p_LZAMS", "p_RZAMS", etc. Presumably these functions are expecting ZAMS values. It may be ok that we pass them "non-ZAMS initial" values instead of ZAMS values, but we should change the parameter names and any text in the descriptions and/or bodies of the functions that imply ZAMS.

• There are a number of functions to which we pass "p_Time", which is the current age of the star (from ZAMS) - is that always known? Presumably this is ok for MS stars, but if we initialise a (e.g.) CHeB star, is the age of the star known (tau is just the age on the phase, isn't it? I assume this is already addressed in the code change for this PR?

• The description of BaseStar::CalculateZAMSAngularFrequency() indicates that the function "calculates initial angular frequency" - if we are overloading "initial", do we need to change that to "ZAMS" (or will it work for "initial" as well?). Should check other functions as well.

• The following functions use "m_*ZAMS" variables directly. How does that work if we haven't initailised a ZAMS star?

  • CHeB::CalculateLambdaDewi()
  • HG::CalculateLambdaDewi()
  • TPAGB::CalculateLambdaDewi()
  • HG::CalculateRadiusOnPhase()

• The descriptions of the following functions indicate that they use constants in a map that are "indexed by ZAMS mass". How does that work if we haven't initialised a ZAMS star? If we can actually index by "non-ZAMS initial" mass, then we at least need to change the descriptions. But can we index by "non-ZAMS initial" mass?

  • CHeB::CalculateLambdaNanjingEnhanced()
  • CHeB::CalculateLambdaNanjingStarTrack()
  • EAGB::CalculateLambdaNanjingStarTrack()
  • HG::CalculateLambdaNanjingStarTrack()
  • TPAGB::CalculateLambdaNanjingStarTrack()

• A number of functions call (e.g.) HeMS::CalculateRadiusAtZAMS_Static() and HeMS::CalculateLuminosityAtZAMS_Static(). Is HeMS at ZAMS impacted if we don't initialise a ZAMS star (or, actually, a HeMS star)?

• We have this in BaseBinaryStar.h:
  "Vector3d m_SystemicVelocity; // Systemic velocity vector, relative to ZAMS Center of Mass"

  Do we know the ZAMS centre of mass if we don't initialise ZAMS stars? If not, is that a problem here? Do we need to change the description - and check if we have similar descriptions for other variables?

As I said, this was just a cursory look, and I don't know how much of this has already been addressed. We should do more than just a cursory look. We need to make sure the code, including descriptions of functions and descriptive comments, is consistent.

[jeffriley]

@jeffriley you mentioned wanting to create a new STELLAR_TYPE for "MainSequence" without a mass specification.
This seems to me to be a bit of overhead, and outside the scope of what I can do. If you still feel it is important to add,
could you make the relevant changes for this?

Actually I think that was @ilyamandel, but I agree. The coding is trivial - making it fit with the existing stellar-type hierarchy in a > sensible way (and that doesn't interfere with evolution/checks later) will need some thought. I can take a look at that.

Scratch my answer :-) I don't think we need a new stellar type - I think we can just use an "MS" mnemonic for the option value, and we initalise the star as one of the MS types (based on mass). As long as we record the option values (and we do), we can still reconstruct what the user did.

[jeffriley]

We could maybe put a warning into the splash screen to mention this
(e.g. "Hey, you are initializing non-ZAMS stars, make sure you specify the mass and period distributions")

If the user is using a grid file, we won't know at the time the splash screen is written that they are initialising non-ZAMS stars.
The warning would have to come later, and I think on a per-star/binary basis.

We should enforce any of these dependencies. There will likely be others - if the user is starting evolution of a star post-ZAMS I can imagine that some options are not going to be allowed (as opposed to just not applicable - not applicable we can silently ignore, but not allowed we should issue an error and abandon evolution of that star/binary).

[jeffriley]

I think some (many?) of the coding issues around "ZAMS" vs "initial" can be mitigated by:

• creating new variables in the code for "initial" values (say, "*_INITIAL")
• the options that set initial values for non-ZAMS stars are the same ones used to set ZAMS values for MS stars (i.e. ZAMS values for MS stars are just their initial values)
• when a star is initialised as an MS star, its ZAMS values are recorded in "*_ZAMS" variables, and also in "*_INITIAL" variables
• when a star is initialised as a non-ZAMS (post-MS) star, its ZAMS values are recorded as -1 (or some other sentinel) in "*_ZAMS" variables, and its initial values are recorded in "*_INITIAL" values
• wherever we currently use "*_ZAMS" values, we change the code to use "*_INITIAL" values if appropriate. Where not appropriate (i.e. we require ZAMS values), we check that we have valid ZAMS values and fail (and abandon evolution) if we don't.

[jeffriley]

And let's not forget the documentation. We have to describe this new paradigm in the documentation, and then check everywhere we mention ZAMS in the documentation whether we need to change "ZAMS" to "initial" (and make sure the users know what we mean by "initial"). We should document anomalies and dependencies (especially those that we enforce).

(I've said this before...) This is not a minor change to functionality - not only is it a shift in the way COMPAS evolves stars and binaries, it is a major change to the code (i.e we move the version number to v03.00.00 with implementation of this PR).

[reinhold-willcox]

Superceded by #1156.