TeamCOMPAS / COMPAS

COMPAS rapid binary population synthesis code
http://compas.science
MIT License
64 stars 66 forks source link

Changes to timesteps, mass transfer timescale, other minor tweaks #1123

Closed ilyamandel closed 4 months ago

ilyamandel commented 4 months ago

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 commented 4 months ago

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

ilyamandel commented 4 months ago

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

ilyamandel commented 4 months ago

Closing #1114 .

ilyamandel commented 4 months ago

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

ilyamandel commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

ilyamandel commented 4 months ago

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.