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

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