Changes to timesteps, mass transfer timescale, other minor tweaks

[ilyamandel]

Added options --radial-change-fraction and --mass-change-fraction, as approximate desired fractional changes in stellar radius and mass on phase when setting SSE and BSE timesteps

The recommended values for both parameters are 0.005, but the default remains 0, which reproduces previous timestep choices

Mass transfer from main sequence donors (including HeMS) can now proceed on nuclear timescales -- approximated as the radial expansion timescales -- if equilibrium zetas are greater than Roche lobe zetas

Removed the fixed constant MULLERMANDEL_MAXNS; instead, OPTIONS->MaximumNeutronStarMass() is used for consistency (see issue #1114)

Corrected return units of CalculateRadialExpansionTimescale() to Myr

Updated documentation

Minor code cleanup

[ilyamandel]

May address #1076 (but not tested specifically for accretion onto NS, so not declaring victory yet).

[ilyamandel]

Should address #889 , leaving open for now as a reminder to test explicitly.

[ilyamandel]

Closing #1114 .

[ilyamandel]

Closing #470 -- arbitrarily smooth HR diagrams can now be produced with this PR.

[ilyamandel]

I hoped to address #24 with this one (convergence checks and adaptive time steps), but only partially succeeded.

Convergence is almost guaranteed (though not strictly ensured, since these are estimated changes for future time steps, not guaranteed changes in past time steps) by setting --mass-fractional-change and --radial-fractional-change to desired values, such as 0.005. Of course, that does carry a significant computational cost, so adaptive time stepping is still desirable.

I did attempt to increase the time step adaptively (by a factor of 2 over previous time step) if not counter-indicated by radial or mass change timescales, but that did sometimes lead to over-runs (e.g., SSE starting with a 6 Msun star could leave a massless remnant). Leaving this out for this PR -- a project for the future.

[jeffriley]

Thanks @ilyamandel. Just a few comments/suggestions:

1. You should add the new options to yaml.h. yaml.h is the template for the yanl file production (via the --create-yaml-file option). As you have seen, it's not absolutely necessary to update yaml.h - the code will add the new options to the end of the yaml file - but updating yaml.h with the new options locates them in (hopefully) a logical part of the yaml file so people can locate them easily.
2. The new options should both be in the m_RangeExcluded and m_SetExcluded vectors (at the top of Options.h - full Instructions for adding a new option are at the top of Options.cpp - can you check that you did all the steps required?)
3. Remove commented line in BaseStar::UpdateAttributesAndAgeOneTimestepPreamble()
4. Since we no longer use the LimitTimestep() function it should be removed - unless you're thinking you mightreinstate it? Even then, you can get it from a previous version. Should the documentation explicitly state the 1% mass loss limit is being replaced by a user-defined limit via --mass-change-fraction? Should that be made clearer in What's New?
5. It looks like the timestep you calculate in BaseStar::CalculateTimestep() is the timestep required to meet the change requirement(s) in isolation. So there is no interaction/combination of the two new options, and the timestep calculations in BaseStar::CalculateTimestep() don't take any limiting of mass loss (e.g. photon tiring) into account? Should we document that?
6. Should there be maximum allowed values for the --mass-change-fraction and --radial-change-fraction options? If not, what's the effect of very large values? I suppose we just lose up to the maximum the star is capable of losing - so the value given by the user is a maximum? (also relates to photon tiring limit I think).
7. How does the --timestep-multipler option interact with each of the --mass-change-fraction and --radial-change-fraction options?
8. Ok - just saw the "approxinate goal" comment in the code (and the text of the option descriptions in the docs). Maybe "default" timesteps rather than "previously suggested" timesteps (in the docs decriptions)?
9. Should we have a page in the documentation describing the effect of the new options, and the --timestep-multiplier option, and maybe some of the above? Maybe a "Timestepping" page in the User Guide somewhere?
10. How does --mass-change-fraction interact with the constant MAXIMUM_MASS_LOSS_FRACTION (wherever it is used in the code)? Is the maximum mass loss fraction still defined by MAXIMUM_MASS_LOSS_FRACTION (so is the value for the new option limited by MAXIMUM_MASS_LOSS_FRACTION)? So should we actually limit the allowed value for the option?
11. How does --radial-change fraction interact with MAXIMUM_RADIAL_CHANGE, and so Star::EvolveOneTimestep() (for SSE mode)? Ditto for this - is the new option limited by MAXIMUM_RADIAL_CHANGE? So should we actually limit the allowed value for the option?

[jeffriley]

It looks like the timestep you calculate in BaseStar::CalculateTimestep() is the timestep required to meet the change
requirement(s) in isolation. So there is no interaction/combination of the two new options

No, that's wrong - there is interaction. Does one take precendence over the other? You possibly limit by the radial fraction first, then possibly by the mass fraction - results could be different if you switched those, couldn't they? Maybe not...

[ilyamandel]

Thank you for the review, @jeffriley !

1, 2, 3, 4 addressed.

The second part of 4, 10, 11 (mass and radius change fractions) are not impacted: these are hard limits calculated post factum, while I am adjusting timestep estimates before taking the timesteps.

To make these educated guesses (that's all they are), I use mass and radius evolution scales estimated based on the previous timestep. If these were impacted by photon tiring, then that's automatically included, I intentionally don't delve into the details. (That's my answer to 5.)

6: very large values for --mass-change-fraction or --radial-change-fraction, these end up being ignored when estimating the next timestep (i.e., similar behaviour as when the user picks zero).

7: TimestepMultiplier is applied after the timestep is estimated, so effects are cumulative. Clarified in documentation.

8: I didn't like "previously suggested", either, but defaults might eventually change, so I didn't want to mention default. I've changed to "A value of 0 means that this choice is ignored and timestep estimates from earlier COMPAS version are used."

9: Maybe eventually, but I'll wait until we have an even more permanent solution to #24 [this is better than nothing, but it's not the final version].

10, 11: See above.

Ready for re-review.

[ilyamandel]

Updated pre-processing-sampling.rst to not mention Stroopwafel for now (see #1088 ).

[ilyamandel]

Addressed #1016: added option --natal-kick-for-PPISN; if set to true, PPISN remnants receive the same natal kick as other CCSN, otherwise (default) they receive no natal kick.

[jeffriley]

Thanks @ilyamandel - looks good