Conversation
The new argument takes a list of arguments to pass to the alignment tool. This gives more flexibility in configuring mafft.
- Found a bug, where I accidentally removed "threads" from the argument string
- tests just show the command line mafft call is what we expect - Clarified the docs that --keeplength is automatically added when an existing alignment is passed
- I modified the mafft command string to use newer python format strings - Made both paths (with and witout existing file) use the same command, with slightly different arguments - Updated the tests slightly, as it was difficult to unify the commands without a bit of reordering
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1790 +/- ##
==========================================
+ Coverage 73.35% 73.37% +0.01%
==========================================
Files 81 81
Lines 8535 8547 +12
Branches 1735 1738 +3
==========================================
+ Hits 6261 6271 +10
- Misses 1980 1981 +1
- Partials 294 295 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Changed the behaviour to not override default arguments (and expand instead) unless the --override-defaults-args parameter is set to true. - Updated tests to reflect this change.
victorlin
left a comment
victorlin
left a comment
There was a problem hiding this comment.
Thanks for adding --override-default-args! That part looks good.
Apologies for the delayed review and not catching this requested change earlier.
| files_to_align = f"--add {shquote(seqs_to_align_fname)} {shquote(existing_aln_fname)}" | ||
| alignment_args = " ".join(["--keeplength", alignment_args]) | ||
|
|
||
| cmd = f"mafft {alignment_args} --thread {nthreads} {files_to_align} 1> {shquote(aln_fname)} 2> {shquote(log_fname)}" |
There was a problem hiding this comment.
It should be checked that alignment_args does not conflict with the hardcoded options (--add, --keeplength, --thread). See usage of augur.tree.check_conflicting_args as an example of how this is done in augur tree.
Actually, I think augur.tree.check_conflicting_args can be repurposed and used here by moving into augur.utils and replacing the "tree builder" references with something like "external program", but that's extra work. It's fine to copy the function as augur.align.check_conflicting_args.
3 participants
The new argument takes a list of arguments to pass to the alignment tool. This gives more flexibility in configuring mafft.
Description of proposed changes
Added the new argument to
align.py. If present, the arguments override the defaults (except threads which is passed separately).Related issue(s)
#1789
Checklist
./run_tests.sh -k test_align.py