Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nuclear timescale calculation makes assumptions about core mass #430

Open
TomWagg opened this issue Nov 5, 2020 · 6 comments
Open

Nuclear timescale calculation makes assumptions about core mass #430

TomWagg opened this issue Nov 5, 2020 · 6 comments
Labels
good first issue Good for newcomers severity_minor This bug is not very severe urgency_low This issue is not urgent

Comments

@TomWagg
Copy link
Collaborator

TomWagg commented Nov 5, 2020

Describe the bug
Whilst plotting the nuclear timescale with Selma we noticed that it seemed off by a factor of ~3-5 from what it should be for high mass stars. The timescale doesn't seem to be used in any meaningful way except printing so only problem would be when people are using this for other things. The equation used by COMPAS is assuming that core mass makes up a certain fraction of the total mass, so it works for solar mass stars but once you get to higher mass stars things get iffy.

Label the issue
urgency_low,severity_moderate

To Fix
See BaseStar::CalculateNuclearTimescale_Static. This uses the last expression of Kalogera & Webbink 1996 Eq.3, but if you look at this equation in the text you see you should really be using the core mass expression, not using the second expression
image

@TomWagg TomWagg added bug Something isn't working severity_moderate This is a moderately severe bug urgency_low This issue is not urgent labels Nov 5, 2020
@TomWagg TomWagg changed the title Nuclear timescale calculation has lots of assumptions Nuclear timescale calculation makes assumptions about core mass Nov 5, 2020
@SimonStevenson
Copy link
Collaborator

I'm shocked that we even have this. I suggest simply removing it, since time-scales should be self consistently calculated from the stellar models (currently SSE)

@TomWagg
Copy link
Collaborator Author

TomWagg commented Nov 5, 2020

Okay gotcha, in that case would CalculateDynamicalTimescale also be up for the chop?

@SimonStevenson
Copy link
Collaborator

We could certainly make your fix, just seems odd that we have this if we don't use it. I think we may actually use the dynamical timescale for something (we can check); if so we better not remove that

@reinhold-willcox
Copy link
Collaborator

I think the dynamical timescale is just used as a lower bound. We also have a hardcoded minimum timescale, so I believe the larger of these two at any given timestep is taken as the lower bound.

@ilyamandel
Copy link
Collaborator

This is meant to be a crude estimate, for potential use only when crude estimates are useful (e.g., when deciding on appropriate time step sizes). We can remove it, but I don't see much harm. I don't think we can make the fix @TomWagg suggests, though: there's no core mass as such for MS stars. A more relevant simple fix to be to change the energy efficiency for more evolved stars -- the helium mass deficit is lower than the hydrogen much deficit. :)

In any case, I don't think this is a particular issue provided this is only viewed as a crude estimate... But since we don't use it except during CEE printing (where it's typically very incorrect because these are evolved stars that are fusing He, not because the core mass fraction is wrong), let's just remove it.

The dynamical timescale estimate is correct as far as I can tell, so no reason to change that.

@ilyamandel ilyamandel added severity_minor This bug is not very severe and removed bug Something isn't working severity_moderate This is a moderately severe bug labels Nov 8, 2020
@ilyamandel
Copy link
Collaborator

As a reminder, the proposed solution is to remove BaseStar::CalculateNuclearTimescale_Static and remove it from output.

@ilyamandel ilyamandel added the good first issue Good for newcomers label Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers severity_minor This bug is not very severe urgency_low This issue is not urgent
Projects
None yet
Development

No branches or pull requests

4 participants