-
Notifications
You must be signed in to change notification settings - Fork 66
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 #1156
base: dev_frozen_for_comparison_with_genzams_changes
Are you sure you want to change the base?
Non-ZAMS Initialization #1156
Conversation
…rt it to stellar type for getters
…tial stellar type
…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).
…hese are superceded by the same parameters with the suffix _CGS
…_PROPERTY::TIME in the BSE_DETAILED_OUTPUT_REC output
…ostly just a regex search/replace, so it is likely broken, but it at least compiles
…s vs MZAMS (and similar for the other initial atts)
… variables related to initial mass, radius, temperature and luminosity (including STAR_PROPERTIES and related functions), as distinct from the ZAMS variables, so that both can be used depending on the context
… sets ZAMS values to -1 (for debugging purposes)
Major edits:
|
@reinhold-willcox -- do you want us to formally review a merge from one branch into another (neither of them being dev, so no review required), or just give you comments on this? |
We have m_OmegaZAMS, and BaseStar::CalculateZAMSAngularFrequency() that calculates omega at ZAMS using Hurley et al. 2000, eq 108. Does Hurley et al. 2000, eq 108 generalises beyond ZAMS?
Why not implement the complete solution now? All it requires is the user providing a couple of values that we don't otherwise know - that's not difficult to do. Where the user gets those values is not really our concern (though they could easily get them from prior COMPAS runs) - as long as we require they provide them.
Indeed - so in follow-up to @ilyamandel's question above - what is it you're looking for here? Any review will surely point out what you've just pointed out, so perhaps it is worth you doing that work before we review? |
@ilyamandel and @jeffriley I will eventually be looking for a formal review, even though the target branch is not @jeffriley The reason to avoid doing the complete solution now is that it's quite a bit more work for me. I was hoping to avoid scope creep (though that already seems to be happening with this Initial/ZAMS discussion). |
@reinhold-willcox ok, I'll take a look when I can. I think the work to implement the complete solution is minor compared to the work to introduce what you will be implementing. Let me take a look and see if I'm right. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things that stood out from a quick skim (not a review):
BaseStar.cpp:
m_MZAMS = CalculateRadiusAtZAMS(m_MZAMS);
This looks wrong. :)
BaseStar.h, etc:
virtual void FastForward() { return; }
An ambiguous function name.
You require that stellar types with an initial fractional lifetime parameter tau are used as initial types, but you don't seem to ask for tau as an input.
This is the start of a major refactorization in COMPAS in which we will allow non-ZAMS initialization of stars. This is related to PRs #931 and #1036 (and supercedes both). In the latter, it was suggested that we need to be very careful with the naming convention of variables that use either "ZAMS" or "initial" (or some variation), since these were previously interchangeable, but now have distinct meanings. This PR is the start of that effort to split apart the usage of ZAMS and Initial parameters, and includes the desired non-ZAMS initialization functionality.
This is expected to be a long-term PR, with input from different people at various points. Keeping this version up to date with the other changes in
dev
will become unmanageable. Thus, I have created a separate branchdev_frozen_for_comparison_with_genzams_changes
, which is a version ofdev
that will not be updated, but can be used for comparison with the changes in this branch. It will be easier to compare the major changes here to a frozen version ofdev
while we are iterating on this. Once we are happy with this major refactorization, I'll do a subsequent merge to bring these changes up to date with the concurrent updates indev
.The major edits are described explicitly below.
I would like to get feedback from @ilyamandel @jeffriley and @avigna, specifiically in response to your previous requests, though others are encouraged to contribute as well.