fix(tree): check for all synonyms of conflicting default args #1547
Conversation
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`
If `-redo` is not added, one can get errors like this: ``` > --tree-builder-args="-czb -bb 1000 -bnni" \ > --override-default-args \ > --output "$TMP/tree_raw.nwk" > /dev/null + + ERROR: Shell exited 2 when running: iqtree2 --threads-max 1 -s tree/full_aligned-delim.fasta -m GTR -czb -bb 1000 -bnni > tree/full_aligned-delim.iqtree.log + Command output was: + Checkpoint (tree/full_aligned-delim.fasta.ckp.gz) indicates that a previous run successfully finished + Use `-redo` option if you really want to redo the analysis and overwrite all output files. + Use `--redo-tree` option if you want to restore ModelFinder and only redo tree search. + Use `--undo` option if you want to continue previous run when changing/adding options. + + [1] ```
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1547 +/- ##
==========================================
+ Coverage 70.09% 70.13% +0.03%
==========================================
Files 75 75
Lines 7923 7923
Branches 1938 1938
==========================================
+ Hits 5554 5557 +3
+ Misses 2085 2082 -3
Partials 284 284 ☔ View full report in Codecov by Sentry. |
|
Would be nice if someone could look at this, e.g. @huddlej @victorlin @tsibley @genehack @joverlee521 @jameshadfield |
| @@ -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, ( | |||
There was a problem hiding this comment.
not at all familiar with this code, ignore if I'm just utterly confused
why are these args hard-coded in place here, instead of being part of the DEFAULT_ARGS data structure around L25? digging a little bit, I see this is a pattern throughout, but I find it sort of confusing that there's a set of hardcoded default arguments for each type of tree-builder, and then we're checking for "conflicts with hardcoded defaults" when running each tree-builder, using a different set of arguments.
There was a problem hiding this comment.
I think hard-coded args are arguments that are set controlled through augur tree options and for which we want to disallow users from overwriting them. Things like: the input alignment, which augur tree processes and sets directly, there's no reason why a user should change that, we want to make this part of the underlying tree binary "private".
Default args are recommendations for tree builder programs that user may override if they want to without causing harm.
Does that make sense?
There was a problem hiding this comment.
Does that make sense?
It does; albeit the use of "default" to mean two orthogonal things that look like they're different ways of saying the same thing (DEFAULT_ARGS versus "hardcoded defaults") seems inherently confusing.
Maybe "hardcoded defaults" could be changed to "hardcoded options", but this is a nitpick. 🤷
| > --override-default-args \ | ||
| > --output "$TMP/tree_raw.nwk" > /dev/null | ||
|
|
||
| Build a tree with conflicting default arguments. |
There was a problem hiding this comment.
for the sake of symmetry, might be worth adding one additional test that is identical to this plus --override-default-args -- or instead having this test duplicate the arguments in the test above without --override-default-args.
There was a problem hiding this comment.
These cram tests are the slow part of the test suit so I'd probably prefer not to add tests just for symmetry sake - if the tests were fast, then sure!
There was a problem hiding this comment.
Thanks, @corneliusroemer! Nice to have the check for modern synonyms and also clearer to use those throughout the docs and tests (-czb has always confused me, for example, and --polytomy is much clearer).
| @@ -49,10 +49,21 @@ Since the following custom arguments are incompatible with the default IQ-TREE a | |||
| $ ${AUGUR} tree \ | |||
| > --method iqtree \ | |||
| > --alignment tree/full_aligned.fasta \ | |||
| > --tree-builder-args="-czb -bb 1000 -bnni" \ | |||
| > --tree-builder-args="--polytomy -bb 1000 -bnni --redo" \ | |||
There was a problem hiding this comment.
Is the addition of --redo necessary here because we're writing the output of all these tests to $TMP? If so, we could just remove references to $TMP throughout this file (following our own cram style guide) and skip the --redo.
There was a problem hiding this comment.
I'm not quite sure anymore why I needed to add the --redo - there was definitely some issue I hit without it - possibly iqtree stubmling upon an existing file from an earlier test without having cleaned up.
Redo does no harm (it should be the default, and one should actively opt into checkpoints). It might not bring many benefits but given there's no harm I thought I'd sprinkle it.
There was a problem hiding this comment.
This would require reorganizing the cram files: #1558
I plan to make a PR for that later, but this shouldn't block on it!
There was a problem hiding this comment.
Actually I'm not sure we can avoid --redo. I realized while working on #1559 that IQ-TREE puts intermediate files in the directory of the --alignment input file, not the directory of the --output file. Only the 2nd is cleaned up after every test.
-redoto iqtree argsDescription of proposed changes
See commit messages for details.
Related issue(s)
Inspired by iqtree/iqtree2#273
Checklist