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

CLI docs have incorrect default values for options using store_false action #1585

Open
Tracked by #1642
victorlin opened this issue Aug 20, 2024 · 2 comments
Open
Tracked by #1642
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@victorlin
Copy link
Member

victorlin commented Aug 20, 2024

Scope

Any CLI option that uses the store_false action, which are these options on version 25.2.0:

  • augur curate format-dates --no-mask-failure
  • augur filter --no-probabilistic-sampling
  • augur refine --no-covariance
  • augur refine --greedy-resolve
  • augur mask --no-cleanup

Issue

The CLI docs show a default value of True for these options. I noticed this because it was confusing on the CLI docs for augur filter, where --probabilistic-sampling and --no-probabilistic-sampling are supposed to be mutually exclusive:

image

augur filter --help text does not contradict, but there is no text provided for --no-probabilistic-sampling which still isn't great:

  --probabilistic-sampling
                        Allow probabilistic sampling during subsampling. This is useful when there are more groups than requested sequences. This option only applies when `--subsample-max-sequences` is
                        provided. (default: True)
  --no-probabilistic-sampling

and then I realized that the lack of help text may be intentional to avoid the same issue:

parser.add_argument('--no-covariance', dest='covariance', action='store_false') #If you set help here, it displays 'default: True' - which is confusing!

Possible solutions

  1. Describe defaults in help text and avoid printing any defaults according to argparse. I think this is what Nextstrain CLI does for its arguments that use store_false such as nextstrain build --no-download.
  2. Use store_true and flip the boolean internally. Something similar was done in Clarify default values for inference of ambiguous bases #613
@victorlin victorlin added bug Something isn't working documentation Improvements or additions to documentation labels Aug 20, 2024
@corneliusroemer
Copy link
Member

I'm pretty sure I remember noticing this in the past - but I must have forgotten to make an issue. Thanks!

@joverlee521
Copy link
Contributor

Ah, I thought I fixed this issue for augur curate format-dates --no-mask-failure with 4d02220.

Looks okay in the CLI

augur curate format-dates -h
...
  --no-mask-failure     Do not mask dates with 'XXXX-XX-XX' and return original date string if date formatting failed. (default:
                        False)

But I'm seeing that this can be confusing in the docs
Screenshot 2024-08-20 at 10 43 10 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants