fix(tree): check for all synonyms of conflicting default args (#1547)

IQtree2 has multiple synonyms for default args

We were only checking for one of the synonyms. This PR fixes that.

It also uses the currently preferred name for all IQtree options, where "preferred" is defined as the name shown in `iqtree2 -h`

List of changes:
`-nt` -> `-T`
`--ntmax` -> `--threads-max`
`-czb` -> `--polytomy`

List of synonyms:
`-ntmax`==`--threads-max`
`-s`==`--aln`==`--msa`
`-m`==`--model`

Author: corneliusroemer

CHANGES.md

@@ -6,13 +6,15 @@
 
 * 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
 
 * index: Previously specifying a directory that does not exist in the path to `--output` would result in an incorrect error stating that the input file does not exist. It now shows the correct path responsible for the error. [#1644][] (@victorlin)
 * 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
 

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']}".

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]

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

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