Follow-on issues from GW radiation update

[ilyamandel]

There are a few follow-on issues from #1080 (not problems with that PR itself, but things that became apparent while working on it) that I am listing below so we don't forget them:

1. I don't think the following is what we want to do going forward (BaseBinaryStar.cpp):

if ((m_Star1->IsOneOf({ STELLAR_TYPE::MASSLESS_REMNANT }) || m_Star2->IsOneOf({ STELLAR_TYPE::MASSLESS_REMNANT })) || p_Dt < NUCLEAR_MINIMUM_TIMESTEP) {
p_Dt = NUCLEAR_MINIMUM_TIMESTEP; // but not less than minimum
}

We may want to continue evolving merger products (which would be characterised by one component becoming the merger product and the other being a massless remnant). We don't want to take tiny timesteps in that case; on the contrary, the timestep should be driven exclusively by the dt passed back by the star which is not a massless remnant.

2. In retrospect, I think it was a suboptimal implementation decision to equate a negative semi-major axis with the binary being unbound, and we should fix that in the future. Otherwise, we end up having to use hacks such as setting the semi-major axis to small positive numbers as the binary approaches GW-driven merger.

3. I am worried that we may be resetting m_SemiMajorAxisPrev and m_EccentricityPrev multiple times per time step (e.g., what if we have mass transfer and GW emission happening simultaneously?), so these values could end up being overwritten in a way that may not match the developer's or user's expectations. The longer-term solution is to apply all changes to the orbit (from MT, tides, GWs) once per time step.

4. Now that we have a new ChooseTimestep() function in BaseBinaryStar to account for GWs, we should use the same functionality for tides.

[veome22]

@ilyamandel I would be happy to add the timestep code for tides. Quick question-- what would you think about defining global variables such m_DOmega1Dt_tidal, m_DOmega2Dt_tidal, etc. as in the GW code? I've been avoiding it, but it would reduce the number of computations per timestep.

[ilyamandel]

Thank you, @veome22 !
To be honest, I can't recall why global variables were really necessary for either tides or GWs -- why can't the functions that compute them just return them to the caller in a tuple? -- but if you think they are needed, go for it.

[veome22]

Hi @ilyamandel. My current internal implementation of tidal timesteps calls the relevant functions, but the evolution can become painfully slow for the strongest tides. I expect that calculating everything only once and then storing as global variables would increase overhead, but hopefully cut down the run time. I can test both implementations and let you know the time difference, if any.

[ilyamandel]

The fourth item has been addressed, so un-assigning @veome22 since the first three are in my court. :)