TeamCOMPAS / COMPAS

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

Inconsistent radius values #1135

Closed ilyamandel closed 1 month ago

ilyamandel commented 1 month ago

When I ask a MainSequence star that's about to transfer mass (but hasn't done anything yet, just at the start of BaseBinaryStar::CalculateMassTransfer() ) what is its Radius() and compare that to the value of CalculateRadiusOnPhase() evaluated at the same time, they don't match.

[Personally, I would prefer not to store parameters like radius that can be easily recomputed at all, but compute them on the fly when needed; storing them saves computer time, but wastes programmer time when such inconsistencies arise and can lead to incorrect behaviour.]

jeffriley commented 1 month ago

@ilyamandel I can't duplicate this with v02.47.01 - can you check, and if you can tell me the commandline you use?

EDIT: and how you check the radius of the MainSequence star

And...

[Personally, I would prefer not to store parameters like radius that can be easily recomputed at all, but compute them on the fly when needed; storing them saves computer time, but wastes programmer time when such inconsistencies arise and can lead to incorrect behaviour.]

Two things. First, the stored variable will be equal to the computed value if computed in the right place - I don't think you should necessarily expect that at random times during a timestep - and if it's not, yes, we have a problem. Next, I'd balance your frustration (and mine when it happens to me...) against the knowledge that COMPAS runtime performance affects all users all the time, whereas ease of debug affects one programmer occasionally.

ilyamandel commented 1 month ago

I believe the problem was because I was using the current value of tau to compute the radius on phase, but that value was not, in fact, current. I agree with @jeffriley's points above, so closing this issue.