diff --git a/CHANGES.md b/CHANGES.md index 52447c001..1f59dd88a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -6,6 +6,7 @@ * ancestral, translate: Add `--skip-validation` as an alias to `--validation-mode=skip`. [#1656][] (@victorlin) * clades: Allow customizing the validation of input node data JSON files with `--validation-mode` and `--skip-validation`. [#1656][] (@victorlin) +* tree: When using iqtree, check for all synonyms of default args when detecting potential conflicts, e.g. `--threads-max` is equivalent to `-ntmax`. Previously, we were only checking for the latter. Also use new, preferred IQtree2 option names (e.g. `--polytomy` instead of `-czb` etc.). [#1547][] (@corneliusroemer) ### Bug Fixes @@ -13,6 +14,7 @@ * curate format-dates: Update help docs and improve failure messages to show use of `--expected-date-formats`. [#1653][] (@joverlee521) [#1644]: https://github.com/nextstrain/augur/issues/1644 +[#1547]: https://github.com/nextstrain/augur/pull/1547 [#1653]: https://github.com/nextstrain/augur/pull/1653 [#1656]: https://github.com/nextstrain/augur/pull/1656 diff --git a/augur/tree.py b/augur/tree.py index 98c31f510..cbef80840 100644 --- a/augur/tree.py +++ b/augur/tree.py @@ -28,18 +28,18 @@ # For compat with older versions of iqtree, we avoid the newish -fast # option alias and instead spell out its component parts: # - # -ninit 2 + # --ninit 2 # -n 2 - # -me 0.05 + # --epsilon 0.05 # # This may need to be updated in the future if we want to stay in lock-step # with -fast, although there's probably no particular reason we have to. # Refer to the handling of -fast in utils/tools.cpp: # https://github.com/Cibiv/IQ-TREE/blob/44753aba/utils/tools.cpp#L2926-L2936 - # Increasing threads (nt) can cause IQtree to run longer, hence use AUTO by default - # Auto makes IQtree chose the optimal number of threads - # Redo prevents IQtree errors when a run was aborted and restarted - "iqtree": "-ninit 2 -n 2 -me 0.05 -nt AUTO -redo", + # Increasing threads (-T) can cause IQtree to run longer, hence use AUTO by default + # -T AUTO makes IQtree chose the optimal number of threads + # --redo prevents IQtree errors when a run was aborted and restarted + "iqtree": "--ninit 2 -n 2 --epsilon 0.05 -T AUTO --redo", } # IQ-TREE only; see usage below @@ -71,12 +71,12 @@ def check_conflicting_args(tree_builder_args, defaults): Examples -------- - >>> defaults = ("-ntmax", "-m", "-s") - >>> check_conflicting_args("-czb -n 2", defaults) - >>> check_conflicting_args("-czb -ntmax 2", defaults) + >>> defaults = ("--threads-max", "-m", "-s") + >>> check_conflicting_args("--polytomy -n 2", defaults) + >>> check_conflicting_args("--polytomy --threads-max 2", defaults) Traceback (most recent call last): ... - augur.tree.ConflictingArgumentsException: The following tree builder arguments conflict with hardcoded defaults. Remove these arguments and try again: -ntmax + augur.tree.ConflictingArgumentsException: The following tree builder arguments conflict with hardcoded defaults. Remove these arguments and try again: --threads-max """ # Parse tree builder argument string into a list of shell arguments. This @@ -261,13 +261,27 @@ def random_string(n): ofile.write(tmp_line) # Check tree builder arguments for conflicts with hardcoded defaults. - check_conflicting_args(tree_builder_args, ("-ntmax", "-s", "-m")) + check_conflicting_args(tree_builder_args, ( + # -ntmax/--threads-max are synonyms + # see https://github.com/iqtree/iqtree2/blob/74da454bbd98d6ecb8cb955975a50de59785fbde/utils/tools.cpp#L4882-L4883 + "-ntmax", + "--threads-max", + # -s/--aln/--msa are synonyms + # see https://github.com/iqtree/iqtree2/blob/74da454bbd98d6ecb8cb955975a50de59785fbde/utils/tools.cpp#L2370-L2371 + "-s", + "--aln", + "--msa", + # --model/-m are synonyms + # see https://github.com/iqtree/iqtree2/blob/74da454bbd98d6ecb8cb955975a50de59785fbde/utils/tools.cpp#L3232-L3233 + "-m", + "--model", + )) if substitution_model.lower() != "auto": - call = [iqtree, "-ntmax", str(nthreads), "-s", shquote(tmp_aln_file), + call = [iqtree, "--threads-max", str(nthreads), "-s", shquote(tmp_aln_file), "-m", shquote(substitution_model), tree_builder_args, ">", shquote(log_file)] else: - call = [iqtree, "-ntmax", str(nthreads), "-s", shquote(tmp_aln_file), tree_builder_args, ">", shquote(log_file)] + call = [iqtree, "--threads-max", str(nthreads), "-s", shquote(tmp_aln_file), tree_builder_args, ">", shquote(log_file)] cmd = " ".join(call) @@ -425,7 +439,7 @@ def register_parser(parent_subparsers): parser.add_argument('--vcf-reference', type=str, help='fasta file of the sequence the VCF was mapped to') parser.add_argument('--exclude-sites', type=str, help='file name of one-based sites to exclude for raw tree building (BED format in .bed files, second column in tab-delimited files, or one position per line)') parser.add_argument('--tree-builder-args', type=str, help=f"""arguments to pass to the tree builder either augmenting or overriding the default arguments (except for input alignment path, number of threads, and substitution model). - Use the assignment operator (e.g., --tree-builder-args="-czb" for IQ-TREE) to avoid unexpected errors. + Use the assignment operator (e.g., --tree-builder-args="--polytomy" for IQ-TREE) to avoid unexpected errors. FastTree defaults: "{DEFAULT_ARGS['fasttree']}". RAxML defaults: "{DEFAULT_ARGS['raxml']}". IQ-TREE defaults: "{DEFAULT_ARGS['iqtree']}". diff --git a/tests/functional/tree/cram/iqtree-conflicting-default-args.t b/tests/functional/tree/cram/iqtree-conflicting-default-args.t new file mode 100644 index 000000000..863e1e1ae --- /dev/null +++ b/tests/functional/tree/cram/iqtree-conflicting-default-args.t @@ -0,0 +1,14 @@ +Setup + + $ source "$TESTDIR"/_setup.sh + +Build a tree with conflicting default arguments. +Expect error message. + + $ ${AUGUR} tree \ + > --method iqtree \ + > --alignment "$TESTDIR/../data/aligned.fasta" \ + > --tree-builder-args="--threads-max 1 --msa $TESTDIR/../data/aligned.fasta" \ + > --output "tree_raw.nwk" + ERROR: The following tree builder arguments conflict with hardcoded defaults. Remove these arguments and try again: --threads-max, --msa + [1] diff --git a/tests/functional/tree/cram/iqtree-extend-args.t b/tests/functional/tree/cram/iqtree-extend-args.t index 789abb30d..526ad07b4 100644 --- a/tests/functional/tree/cram/iqtree-extend-args.t +++ b/tests/functional/tree/cram/iqtree-extend-args.t @@ -7,5 +7,5 @@ Build a tree, augmenting existing default arguments with custom arguments. $ ${AUGUR} tree \ > --method iqtree \ > --alignment "$TESTDIR/../data/aligned.fasta" \ - > --tree-builder-args="-czb" \ + > --tree-builder-args="--polytomy" \ > --output tree_raw.nwk > /dev/null diff --git a/tests/functional/tree/cram/iqtree-override-args.t b/tests/functional/tree/cram/iqtree-override-args.t index c4562a02b..7871e481d 100644 --- a/tests/functional/tree/cram/iqtree-override-args.t +++ b/tests/functional/tree/cram/iqtree-override-args.t @@ -8,6 +8,6 @@ Since the following custom arguments are incompatible with the default IQ-TREE a $ ${AUGUR} tree \ > --method iqtree \ > --alignment "$TESTDIR/../data/full_aligned.fasta" \ - > --tree-builder-args="-czb -bb 1000 -bnni" \ + > --tree-builder-args="--polytomy -bb 1000 -bnni" \ > --override-default-args \ > --output tree_raw.nwk > /dev/null