TeamCOMPAS / COMPAS

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

CHeB timestep calculation anomaly #481

Closed jeffriley closed 1 year ago

jeffriley commented 3 years ago

Describe the bug Not a bug (yet), just a question.

While writing a section for the methods paper I noticed that the timestep (dtk) calculated for CHeB stars is one tenth of the value suggested by Hurley et al. 2000. Why is this so?

We seem to follow the timestep calculations from Hurley et al. 2000 for all stellar types except CHeB (see Hurley et al. 2000 section 8). For CheB, dtk is one tenth the value.

This comes from the legacy code - so not, as I first thought, a typo when refactoring. The legacy code has some comments regarding the timestep for CHeB:

else if(STELLAR_TYPE == CORE_HELIUM_BURNING){
    // Messing around with step size
    //dtk = 0.02 * timescales[3]; // tHe
    //std::cout << "suggested dtk = " << dtk << std::endl;
    dtk = 2E-3 * timescales[3];
    dte = timescales[2] + timescales[3] - time; // tHeI + tHe - t
    //std::cout << "current dtk, dte = " << dtk << " " << dte << std::endl;
    if(dtk < dte){
        dtk = 2E-3 * timescales[3];
    }
    // Issues with timestep in CHeB?

So I'm presuming this is intentional - or maybe there was a problem and this was accidentally left like this after some debugging?

Anyway, if this is correct then we can close this issue and leave the code as is. I'm hoping @SimonStevenson or @avigna might be able to shed some light.

ps. This might also explain the lines I commented out when refactoring - these are the lines from above:

    if(dtk < dte){
        dtk = 2E-3 * timescales[3];
    }

Which doesn't make any sense given the code above - that's what dtk is set to anyway. But if dtk was actually 0.02 ... instead of 0.002 ..., then the inclusion of that if statement might make more sense.

Label the issue urgency_low - This issue is not urgent severity_minor - This is a minor bug with minimal impact (impact is probably only to performance)

To Reproduce Steps to reproduce the behavior:

Code inspection

Expected behavior A clear and concise description of what you expected to happen.

Screenshots If applicable, add screenshots to help explain your problem.

Versioning (please complete the following information):

Additional context Add any other context about the problem here.

ilyamandel commented 1 year ago

@SimonStevenson , @avigna -- please see @jeffriley's query above. Otherwise, given that we should globally examine time steps for convergence (e.g., #24 ) and perhaps make them dynamical, I am going to label this as "won't fix".

ilyamandel commented 1 year ago

Closing since nobody objected to the wontfix status.