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

Program option rendering / consistency issue #1278

Open
dumbscholar opened this issue Nov 11, 2024 · 3 comments
Open

Program option rendering / consistency issue #1278

dumbscholar opened this issue Nov 11, 2024 · 3 comments
Labels
documentation Improvements or additions to documentation severity_minor This bug is not very severe urgency_low This issue is not urgent

Comments

@dumbscholar
Copy link
Contributor

In the Program options page,

Under --OB-mass-loss and --OB-mass-loss-prescription, the default value is not specified in a new line although it was indended to be so.

Under --mass-loss-prescription, NONE is specified after ZERO. Elsewhere, the order is the opposite.

Under --critical-mass-ratio-prescription, option ZERO is not described. Is NONE going to be deprecated and replaced by ZERO?

@jeffriley
Copy link
Collaborator

Under --OB-mass-loss and --OB-mass-loss-prescription, the default value is not specified in a new line although
it was indended to be so.

Will fix next time a PR is pushed.

Under --mass-loss-prescription, NONE is specified after ZERO. Elsewhere, the order is the opposite.

NONE is deprecated and will be removed (including from the documentation) at the end of the year anyway.

Under --critical-mass-ratio-prescription, option ZERO is not described. Is NONE going to be deprecated and replaced
by ZERO?

No. In this case, NONE means no prescription specified, so the default will be used. ZERO is not an option here, and should not appear in the documentation - will fix next time a PR is pushed.

@dumbscholar
Copy link
Contributor Author

dumbscholar commented Nov 11, 2024

Thanks for addressing the issues.

I think --pulsational-pair-instability-prescription is applicable only when --pulsational-pair-instability is TRUE. This is not specified in the program options. Furthermore, --pulsational-pair-instability is related to mass loss, so should it not be grouped under "Stellar evolution and winds" in the program options and under "Stellar properties" in the yaml file together with --pulsational-pair-instability-prescription in both places?

In the description of --mass-transfer-fa, it can be mentioned that it's referring to the popular beta parameter.

Also, the choice 'COMPAS' can be renamed to 'WOOSLEY' as there is no program option which mentions its default choice as 'COMPAS'. So the naming in this case is inconsistent with rest of the document.

@jeffriley jeffriley added documentation Improvements or additions to documentation severity_minor This bug is not very severe urgency_low This issue is not urgent labels Nov 11, 2024
@ilyamandel
Copy link
Collaborator

Added comment on pulsational-pair-instability-prescription being relevant only when pulsational-pair-instability is used.
I think it's fine to keep this in the same place as pair instability supernovae.
Added comment on -mass-transfer-fa describing beta.

The only one I haven't done but I agree should probably be done is to change the name of the "COMPAS" PPISN prescription to WOOSLEY. @SimonStevenson , are you OK with that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation severity_minor This bug is not very severe urgency_low This issue is not urgent
Projects
None yet
Development

No branches or pull requests

3 participants