Issue 421: Fix alignment coverage >1.0 and aniM symmetrical behaviour #425
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- note that MUMmer's AlignedBases count does not correct for indels which are not aligned to the other sequence in the pairwise comparison.
This commit adds a jupyter notebook and helper script that should facilitate debugging/development. Also, changes were made that add the reverse nucmer run to the joblist for pyani ANIm comparisons. The reverse nucmer run outputs will likely be incorporated into the database, but possibly not correctly, as shortcuts/simplifications were made, assuming a symmetrical matrix.
indent pyani_orm log output to show it belongs to previous log
The sym keyword was removed by @kiepczi as we are now doing forward/reverse comparisons in MUMmer.
The original test still expected two values to be returned from parse_delta() - the tuple returned now has four entries. In addition, the content of the tuple has changed. It now returns the count of aligned bases from reference and query sequences, then the calculated percentage identity, and the count of similarity errors.
The ANIm calculation has been changed to more closely resemble dnadiff.pl output from the MUMmer package. Also, we now run the pairwise comparison in two directions (A v B, B v A), so the resulting matrix is no longer symmatrical. The old deltadir_result.csv file was generated using the former ANIm %ID calculation method, and assumed that the matrix was symmetrical. This has been replaced by results under the new method, and that have been manually checked to be close to dnadiff.pl output. NOTE: the percentage identities found by pyani anim are not identical to dnadiff.pl output. This is at least in part because dnadiff.pl averages percentage identities from an output .mcoords file, where the percentages are rounded to two decimal places. Over hundreds of fragments, the rounding errors mount up and we consider that our approach - maintaining a single count (across exactly the same alignment fragments) and reporting a single identity value - is less prone to these errors
After updating the method for calculating %ID and genome coverage, we now run forward and reverse comparisions. However, the logger did not accurately reflect the number of jobs to run. This was adressed in this commit by using permutations instead of combinations for the list of genomes.
widdowquinn
added
bug
Support files for specific issues that are not part of the codebase should not be included in the pull request.
This should avoid some accidental inclusions of non-core files to the repo for PRs, etc.
|
I have begun review locally.
|
widdowquinn
reviewed
Apr 8, 2024
pyani/anim.py
Outdated
There was a problem hiding this comment.
Not sure why l.162 was reintroduced (logger argument) - have removed this.
widdowquinn
reviewed
Apr 8, 2024
| @@ -255,10 +255,10 @@ def subcmd_report(args: Namespace) -> int: | |||
| "Query description", | |||
| "Subject ID", | |||
| "Subject description", | |||
| "% identity", | |||
| "% identity (weighted)", | |||
There was a problem hiding this comment.
We should remove the indication of (weighted) as this is not true for all comparison methods. I've done this in a new commit.
widdowquinn
reviewed
Apr 8, 2024
| "% query coverage", | ||
| "% subject coverage", | ||
| "alignment length", | ||
| "query alignment length", |
There was a problem hiding this comment.
As above, the alignment length may not be query alignment length for all methods. We should document what these columns mean for different analysis types.
This config skips assert issues in test files
widdowquinn
approved these changes
Apr 8, 2024
There was a problem hiding this comment.
Thanks @kiepczi - and apologies for the delay. That's quite a large number of lines added. These seem mostly to be in the new test output/fixture files. Everything else seems OK and the tests pass.
peterjc
added a commit
to peterjc/pyani
that referenced
this pull request
Oct 8, 2024
This was fixed in subcmd_plot.py during widdowquinn#425
This was referenced Oct 8, 2024
peterjc
added a commit
to peterjc/pyani
that referenced
this pull request
Oct 29, 2024
See also widdowquinn#425 for subcmd_plot.py and widdowquinn#437 for pyani/scripts/subcommands/subcmd_report.py
21 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request resolves issue #421. After discovering that
pyANIoccasionally reports genome coverage values exceeding 1 and non-symmetrical behavior innucmer, it was agreed to revise the method for calculating both the %ID and genome coverage. The changes implemented in the base code include:nucmer.Type of change
Action Checklist
pyanirepository under your own account (please allow write access for repository maintainers)CONTRIBUTING.md)pytest -vnon-passing code will not be mergedorigin/masterflake8andblackbefore submissionPull requestssection in thepyanirepository