-
Notifications
You must be signed in to change notification settings - Fork 69
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
Updated wind perscriptions #996
Conversation
…er merge, changes broken down with history in updated_winds
Two things that will need updating:
|
note in Options.cpp, that we didn't use, Line 1736 in fe48a6b
|
Yeah, that changed a while ago - we should use the new method in this PR (not difficult) |
…ionValuesFormatted() anyway?
Yes - that way the help text is automatically updated if any of the allowed values are changed - and the help text format is kept uniform with other options. |
VMS, RSG, WR mass loss program options
Thanks, one less spot to change options in the future. I've updated these. |
Sorry, I am offline until October 8, so won't be able to review this -- defer to @jeffriley and @SimonStevenson ! |
in |
Let's make it 02.40.00 - this is new functionality, not a defect repair xx.yy.zz -> xx is (supposed to be) major new release; yy minor new release/new functionality; zz defect repair |
Should I list all the specific perscriptions in the changelog or are just the new program options fine? |
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.
Thanks @jmerritt1. I have a few comments and suggested changes. I'll number them so we can refer to them by number if necessary:
-
prescription
is mis-typed asperscription
in a few places:program-options-list-defaults.rst
BaseStar.cpp
changelog.h
constants.h
Options.h
-
Should we implement an option to set the
VERY_MASSIVE_MINIMUM_MASS
threshold, or is 100Msol either the universally accepted threshold or required for our implementation? -
If you run your version of the executable with
./compas --help
, you will see:Options:
...
--OB-mass-loss
OB mass loss prescription (Options: [], default = 'NONE)
...Note no options listed in
[]
- that's because you missed step 9 in the instructions at the top of Options.cppAlso note that there is a missing single quote after
'NONE
- that needs to be added in Options::AddOptions():Change
`("OB mass loss prescription (" + AllowedOptionValuesFormatted("OB-mass-loss") + ", default = '" + p_Options->m_OBMassLoss.typeString + ")").c_str()`
to
`("OB mass loss prescription (" + AllowedOptionValuesFormatted("OB-mass-loss") + ", default = '" + p_Options->m_OBMassLoss.typeString + "')").c_str()`
and similarly for
RSG-mass-loss
,VMS-mass-loss
, andWR-mass-loss
-
If you run your version of the executable with
./compas --create-yaml-file myyaml
, and look at the resultant yaml file (myyaml
). At the bottom of the file you'll see:### Additional COMPAS options not found in YAML template ### # --RSG-mass-loss: 'NJ90' # Default: 'NJ90' # --VMS-mass-loss: 'NONE' # Default: 'NONE' # --WR-mass-loss: 'BELCZYNSKI2010' # Default: 'BELCZYNSKI2010' # --metallicity-distribution: 'ZSOLAR' # Default: 'ZSOLAR' Options: ['LOGUNIFORM','ZSOLAR']
That's because your changes to
yaml.h
:" --OB-mass-loss: 'NONE' # Default: 'NONE'" " --RSG-mass-loss: 'NJ90' # Default: 'NJ90'" " --VMS-mass-loss: 'NONE' # Default: 'NONE'" " --WR-mass-loss: 'BELCZYNSKI2010' # Default: 'BELCZYNSKI2010'"
are malformed. You need to remove the default values in both places. e.g. change
" --OB-mass-loss: 'NONE' # Default: 'NONE'"
to
" --OB-mass-loss"
and similarly for
RSG-mass-loss
,VMS-mass-loss
, andWR-mass-loss
(see examples above and below your additions inyaml.h
). -
BaseStar::CalculateMassLossRateOBBjorklund2022()
:- function name in description does not match actual function name
- is it worth noting the caveats documented in the description (i.e. "This prescription is calibrated to the following ranges:") in the online documentation for the relevant option? Is it important a user knows that (particularly a user who may just use COMPAS without looking at the code)?
-
BaseStar::CalculateMassLossRateOBVinkSander2021()
:- function name in description does not match actual function name
- function has commented code that should be removed
- equations at lines 2015-2020, 2030-2035, and 2045-2051 have ragged right edges with math operators. Either have a single space between the end of the text and the operator, or line all operators up (like the equation at 1967-1973) - the way it is makes it too hard to read.
- it's easier to read if the comments at the end of code lines are aligned with each other too (for humans maintaining the code - the compiler doesn't care of course...)
-
BaseStar::CalculateMassLossRateOBKrticka2018()
:- function name in description does not match actual function name
log10(m_Metallicity / ZSOL)
isBaseStar::LogMetallicityXi()
-log10()
is expensive, no need to call it if we don't have to (and twice...).
-
BaseStar::CalculateMassLossRateRSGBeasor2020()
has commented code that should be removed. -
BaseStar::CalculateMassLossRateRSGDecin2023()
:-
I would probably just rewrite this as:
return PPOW(10.0, -20.63 - 0.16 * m_MZAMS + 3.47 * log10(m_Luminosity));
but, can't we rewrite that as:
return PPOW(m_Luminosity, -20.63 - 0.16 * m_MZAMS + 3.47);
I think the latter would use fewer compute cycles - the former seems to do an unnecessary
log10()
operation, which isn't cheap.
-
-
BaseStar::CalculateMassLossRateRSGKee2021()
:- has commented code that should be removed.
- also, change
2.
->2.0
,17000. -> 17000.0
, and60000. -> 60000.0
(for the same reason as above - easier for humans to read (esp. at a glance))
-
BaseStar::CalculateMassLossRateRSGVinkSabhahit2023()
:- change
-8.
->-8.0
, and-24.
->-24.0
(same as above)
- change
-
BaseStar::CalculateMassLossRateVMSBestenlehner2020()
:- change
1.
->1.0
(same as above) - change constants
1
and2
to1.0
and2.0
. c++ is a strongly-typed language: "1" is an integer, whereas "1.0" is a double. It may make no difference now, but it might if someone changes things later - it's good practice to use the correct data type even for constants.
- change
-
BaseStar::CalculateMassLossRateVMSVink2011()
:- reference/link to paper in the description (is 'Vink 2011' sufficient)?
- function should be
const
- if the
else
clause is executed,rate
is calculated, and is recalculated after theif/else
usinglogMdotdiff
which is undefined (only defined in theif
clause is executed).teff
andrate2001
only need to be calculated in theelse
clause, so those calculations should be moved into theelse
clause. Bring the final calculation ofrate
up into theif
clause (it requireslogMdotdiff
).
-
BaseStar::CalculateMassLossRateVMSSabhahit2023()
:- reference/link to paper in the description (is 'Sabhahit 2023' sufficient)?
- function should be
const
- resolve and remove question on line 2260
-
BaseStar::CalculateMassLossRateWolfRayetSanderVink2020()
:- change the conditional on line 2425 to use utils::Compare()
- Mdot has already been initialised to
0.0
(at line 2412), so no need to set it0.0
at line 2426. Instead, change the conditional to
if (utils::Compare(logL0, logL) < 0)
and delete lines 2426 and 2427. - move line 2434 into the
if
- no need to multiply by the option value if Mdot is0.0
- just wastes compute cycles. - change
- 1
to- 1.0
on line 2430
-
BaseStar::CalculateMassLossRateVMS()
has commented code that should be removed -
BaseStar::CalculateMassLossRateWolfRayetTemperatureCorrectionSander2023()
:- change
141E3
to141.0E3
on line 2457 (same as above re data type) - change
100E3
to100.0E3
on line 2458 (same as above re data type) - change the conditional on line 2465 to use Utils::Compare()
- change
-
BaseStar::CalculateMassLossRateHeliumStarVink2017()
: I think I'd just use a single return line, but up to you. -
BaseStar::CalculateMassLossRateHeliumStarShenar2019()
has commented code that should be removed -
BaseStar::CalculateMassLossRateOB()
: parameter should beconst
-
BaseStar::CalculateMassLossRateRSG()
: parameter should beconst
; function should beconst
-
BaseStar::CalculateMassLossRateVMS()
: function should beconst
-
BaseStar::CalculateMassLossRateUpdatedPrescription()
:- "The structure is similar to above" - do you mean similar to
BaseStar::CalculateMassLossRateVink()
? If so, say so (it may not be above if the code gets changed/refactored).
- "The structure is similar to above" - do you mean similar to
-
Add an entry to the
What's New
page of the online documentation to describe any changes to default behaviour (e.g. which mass loss prescription is now default? Any other changes to default behavour?) -
Your question: "Should I list all the specific perscriptions in the changelog or are just the new program options fine?"
- Just the new options are fine for changelog, but you should add a more complete descripion in the 'What's New' entry (see above). -
Given the number of times we use
log10(m_Luminsoity)
, I wonder if it's worth calculating that whenm_Luminosity
is calculated, and storing it in a class member variable (call itm_Log10Luminosity
- similar tom_Log10Metallicity
)?
That's about it - there are some minor formatting changes I'll either point out later or just fix myself at the final review.
I have not checked the functionality - I checked that the code runs and data is produced, but I have not checked if the contents of the data files are correct according to the new prescriptions added, or whether any existing functionality has been negatively impacted (wrt the correctness of the data produced). I assume you have satisfied yourself that the data produced is correct.
And...
I think that's just the way Read the Docs formats - the line break just happens to be there. You could play around with the text to shift the line break away from the bracket if it really bothers you. |
One more change: Change
in constants.h to
|
Wait...
No, that's wrong (I mentally put parentheses around |
…g file (dont use the method paper config)" This reverts commit 346e98d.
Thanks everyone for your work on this. I've been travelling for two weeks but just got back online and caught up with the discussion.
I've checked again that
I changed this list to include broader types of mass loss, and not specific prescriptions. We are leaning towards only including types, eg. RSG, VMS, etc. I agree that Regarding C. and fWR, I've been running with fWR=1.0 for everything so far, and hadn't realized the default .yaml had changed. |
@SimonStevenson -- I am trying to match up your latest plot with the one from your paper with Teagan. That one was in units of luminosity rather than mass on the abscissa -- but it seemed to show that at the highest luminosities (i.e., high masses), mass loss rates from newer models were actually below those from the old models with a rescaling factor of 0.1, right? |
@jmerritt1 -- I like your suggestion of removing VINK and replacing all of NIEUWENHUIJZEN_DE_JAGER, KUDRITZKI_REIMERS, and VASSILIADIS_WOOD with RSG. Thanks for doing that! |
@ilyamandel We're not using Vink2017 for high-mass (high luminosity) WR stars, only for low-mass (low-luminosity) stripped He stars. At solar metallicity, the transition is around L = 10^5 Lsol (which corresponds to an initial helium star mass somewhere in the range 5-10 Msol). We are using the prescription from Sander & Vink 2020 for high-mass He stars. This results in similar mass-loss rates to the old Belczynski+2010 prescription. |
Thanks for the explanation, @SimonStevenson ! I wonder if we ultimately want two separate WR multiplier choices, for Vink 2017 at lower masses and for Sander & Vink 2020 at higher masses? If we did, I might be tempted to use fWRlowmass = 1 and fWRhighmass=0.1 in my runs... But I think the default should be 1 for both, since these are the best available models, and so long as we use just one multiplier, let's keep the default at 1. BTW, does the transition happen exactly at the point where the Vink 2017 and Sander & Vink 2020 curves cross over in your plot? If not, wind mass loss rates would be discontinuous, which is a bit weird. |
Yes, we just use the maximum of the two prescriptions, so the transition is where they cross over. |
OK, then my only two remaining requests are:
|
Ah actually I've forgotten we will need to add a label for evolved/giant stars that are also below the cutoffs for RSG, see the pink lines (KR) in the below figure for example. (colored by dominant mass loss rate) |
And in the above figure, just want to make sure that this is the intended treatment for the gap between 12.5kK and 8kK, around HG, Simon and I came to the understanding that you'd like these to be treated as WR (see short purple line segments around HG)? We revisited the original conversation with Andreas Sander and this seems to be the intention? |
Looks great!
…On Wed, 1 Nov 2023, 12:43 am jmerritt1, ***@***.***> wrote:
@jmerritt1 <https://github.com/jmerritt1> -- I like your suggestion of
removing VINK and replacing all of NIEUWENHUIJZEN_DE_JAGER,
KUDRITZKI_REIMERS, and VASSILIADIS_WOOD with RSG. Thanks for doing that!
Ah actually I've forgotten we will need to add a label for evolved/giant
stars that are also below the cutoffs for RSG, see the pink lines (KR) in
the below figure for example.
Also, is there any opposition to using acronyms, RSG, LBV, in place of
full labels, since this is mainly a diagnostic tool?
[image: image]
<https://user-images.githubusercontent.com/45217727/279538596-3cf765ae-f2f2-4f1c-ab57-0e09341a2e0d.png>
—
Reply to this email directly, view it on GitHub
<#996 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD5BVJBFP7WQPXNUO7ZKJTYCGEJVAVCNFSM6AAAAAA5ITJLVGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBYGE4DENRXGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Just in case you missed it, there's now an option to do that (--create-yaml-file, see https://compas.readthedocs.io/en/latest/pages/User%20guide/Program%20options/program-options-list-defaults.html#options-props-c) |
Just tried this but have it writing the previous default, fWR=0.1. Where should I change this? options.cpp? (edit, just wasn't working because I forgot to recompile before generating). |
@jmerritt1 I don't think you pushed the new version of Options.cpp - with Oh - and |
Okay I think it's fixed now, and I've removed the one in src. |
Made LSOL and LSOLW consistent
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.
Looks good -- thanks for all the hard work, everyone!
@jmerritt1 -- I saw your new issue post via e-mail, but it seems to have disappeared. I suggest posting new issues separately, since people will likely miss them in closed PRs. |
Updated wind perscriptions
* Fix for issue #1022 * Code cleanup * Event strings (#1021) * (compasUtils) bug fix to getSNevents() * (compasUtils) make getEventHistory() work for COMPAS output with unordered seeds * (compasUtils) fix typo * switched to lexsort from argsort to better handle the ordering of multiple events from the same seed * changed list initialization for np array, changed some variable names for clarity * changed np array back to list, since array couldn't handle input of different sizes --------- Co-authored-by: Reinhold Willcox <[email protected]> * Merge pull request #996 from TeamCOMPAS/updated_winds_fresh Updated wind perscriptions * Reapply winds changes from 2.40.00 * fixes to the detailed plotter, including improved robustness to the Roche Lobe, and masking for only end-of-timestep events * added question to CalculateMassLossRateNieuwenhuijzenDeJager --------- Co-authored-by: Jeff Riley <[email protected]> Co-authored-by: Ilya Mandel <[email protected]> Co-authored-by: Mike Lau <[email protected]> Co-authored-by: jeffriley <[email protected]>
Added a new
UPDATED
option to the mass-loss prescriptions. This is a wrapper applying new specific prescriptions on a case-by-case basis. These are selected via new program options forOB
,VMS
,RSG
, andWR-mass-loss
, which each have their own wrapper function selecting from a total of 15 new perscriptions. Previous default behavior is recoverable by specifyingVINK
.Commit history is in the updated_winds branch, which became relatively unmergable. The changes have been collected and added to an up-to-date branch here as Jeff suggested.