From 695c5edd379864ab36f61266d9f86a34e301dad0 Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Wed, 17 Jul 2024 13:51:16 +0200 Subject: [PATCH 1/5] fix(tree): check for all synonyms of conflicting args 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` --- augur/tree.py | 38 +++++++++++++------ .../cram/iqtree-conflicting-default-args.t | 14 +++++++ 2 files changed, 40 insertions(+), 12 deletions(-) create mode 100644 tests/functional/tree/cram/iqtree-conflicting-default-args.t diff --git a/augur/tree.py b/augur/tree.py index 98c31f510..bd39cd624 100644 --- a/augur/tree.py +++ b/augur/tree.py @@ -36,10 +36,10 @@ # 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 -me 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..4d93ad2d5 --- /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] From 8124f180c16dabfa27f2f48e62b7b54f30f2d164 Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Wed, 17 Jul 2024 14:20:32 +0200 Subject: [PATCH 2/5] Add changelog entry ^ Conflicts: ^ CHANGES.md --- CHANGES.md | 2 ++ 1 file changed, 2 insertions(+) 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 From d13592f1d7381dd4c3b789801f85c8f613d6c8d2 Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Wed, 17 Jul 2024 14:28:50 +0200 Subject: [PATCH 3/5] chore(tree): use new, preferred iqtree option names (those mentioned by `iqtree2 -h`) --- augur/tree.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/augur/tree.py b/augur/tree.py index bd39cd624..cbef80840 100644 --- a/augur/tree.py +++ b/augur/tree.py @@ -28,9 +28,9 @@ # 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. @@ -38,8 +38,8 @@ # https://github.com/Cibiv/IQ-TREE/blob/44753aba/utils/tools.cpp#L2926-L2936 # 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 -me 0.05 -T AUTO -redo", + # --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 From 9b9b8646e5b00a1bc08a417a16bf3629d8e1822a Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Sun, 10 Nov 2024 00:26:04 +0100 Subject: [PATCH 4/5] Reapply changes to newly created files --- tests/functional/tree/cram/iqtree-extend-args.t | 2 +- tests/functional/tree/cram/iqtree-override-args.t | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 From c60a97eb4d9ebbfab913e9f0213ce9935b82d58a Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Sun, 10 Nov 2024 16:43:56 +0100 Subject: [PATCH 5/5] Fix test --- tests/functional/tree/cram/iqtree-conflicting-default-args.t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/tree/cram/iqtree-conflicting-default-args.t b/tests/functional/tree/cram/iqtree-conflicting-default-args.t index 4d93ad2d5..863e1e1ae 100644 --- a/tests/functional/tree/cram/iqtree-conflicting-default-args.t +++ b/tests/functional/tree/cram/iqtree-conflicting-default-args.t @@ -9,6 +9,6 @@ Expect error message. > --method iqtree \ > --alignment "$TESTDIR/../data/aligned.fasta" \ > --tree-builder-args="--threads-max 1 --msa $TESTDIR/../data/aligned.fasta" \ - > --output tree_raw.nwk" + > --output "tree_raw.nwk" ERROR: The following tree builder arguments conflict with hardcoded defaults. Remove these arguments and try again: --threads-max, --msa [1]