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

Non zams initialization #931

Closed
wants to merge 28 commits into from
Closed

Conversation

reinhold-willcox
Copy link
Collaborator

Added option and code functionality for the user to set the stellar type they want at Zams. This is useful for anyone working with, e.g, He stars, X-ray binaries, WDs etc. who want to have more control over the initial conditions of their post-MS binary. Due to the non-trivial value of the tau age parameter, we should only allow users to initialize MS, HeMS, WD (any), NS, or BH stars.

…ut), but they are not internally set correctly - the timestep calculations do not use the stellar types, they use the default (min) in BaseStar
…tellarType, now the code seems to run properly
…pe option. Now users submit a string referring to the stellar type
…ing previous stellar type num functionality)
…envelope_allow_radiative_envelope_survive to True to reproduce the methods paper figure
… values even when the initial stellar type changes (so ZAMS values always correspond to H ZAMS).
…om:reinhold-willcox/COMPAS into generalized_zams_stellar_type_fast_forward
@reinhold-willcox
Copy link
Collaborator Author

This PR encompasses and supercedes PR #732, which is now closed.

@SimonStevenson SimonStevenson self-assigned this Mar 14, 2023
@SimonStevenson
Copy link
Collaborator

It's great that you're working on this, I think it would be a very useful feature. I'd be happy to help review this.

@reinhold-willcox
Copy link
Collaborator Author

@SimonStevenson It should be ready to review now, if you have time.

@jeffriley jeffriley self-requested a review March 14, 2023 06:44
Copy link
Collaborator

@jeffriley jeffriley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll review this wrt terminology and code consistency

@reinhold-willcox
Copy link
Collaborator Author

@jeffriley It says you requested changes but I don't see any requests. Is that a placeholder for future requests, or am I just not seeing existing ones?

@jeffriley
Copy link
Collaborator

It's a placeholder - I did it so the PR wouldn't merge until we've made some changes. I will have time to look at this after the end of this week - but I will be travelling for most of May, so may not get much done until I get back at the start of June.

@ilyamandel
Copy link
Collaborator

@jeffriley -- I notice that there is a note from you from April that says you are about to look at this PR. ;-) I am not sure if it's still relevant (@reinhold-willcox ?) but I think we should probably clean up very old PRs one way or another.

@reinhold-willcox
Copy link
Collaborator Author

@ilyamandel This functionality would be very useful in COMPAS, in my opinion, so I would advocate addressing the remaining issues and getting the code committed.

As it stands, the code here (at some point) worked for the initialization of MS, HeMS, WD, NS, and BH. From memory, Jeff and I were debating whether to allow for the initialization of other stellar types, and stars with tau != 0 (e.g stars partway along the HG), but this could be an extension in a later PR. There was also a question about the naming convention of the variables, but this is cosmetic so I am happy to adopt whatever convention works for the majority. If now is the right time to address these issues, I'll go ahead and update this branch to the latest version of dev.

@ilyamandel
Copy link
Collaborator

I am not aware of any other major changes in progress, @reinhold-willcox , just bug fixes -- so I agree now would be a good time to do this, if you have spare cycles. Thank you!

@reinhold-willcox
Copy link
Collaborator Author

Closing: outdated wrt PR #1036

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants