-
-
Notifications
You must be signed in to change notification settings - Fork 24
Add more SubstitutionMatrix operations #60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Alternatively, I'd be happy to make this a subtype of AbstractMatrix. I think it mostly comes down to whether you want to support indexing with integers or not. My guess is "not," and that's the reason these are not subtypes. |
04d9e33
to
58ff6f7
Compare
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
==========================================
+ Coverage 87.64% 88.08% +0.44%
==========================================
Files 16 16
Lines 1052 1133 +81
==========================================
+ Hits 922 998 +76
- Misses 130 135 +5
Continue to review full report at Codecov.
|
SubstitutionMatrix is not a subtype of AbstractArray. This may or may not be intentional, but currently the number of supported operations is fairly minimal. This provides a few more conveniences for users who might want to manipulate them beyond mere lookup tables.
58ff6f7
to
892f01c
Compare
I didn't write this, so I'm speculating, but I'm guessing the reason the wasn't implemented previously is that substitution matrices tend to be pretty static, and don't require much beyond being able to reference a particular value. Things like BLOSUM have been around for decades more or less unchanged. That said, I can't think of any reason NOT to do this, other than the up-front effort that you've already expended. So I'm in favor of merging. Additional maintenance burden seems trivial. |
Out of curiousity, do you happen to know which version is implemented? https://www.nature.com/articles/nbt0308-274 |
We have all of them - I think they're just downloaded direct from NCBI: https://biojulia.net/BioAlignments.jl/dev/pairalign/#Substitution-matrix-types-1 |
Right, I just meant, does NCBI host the original or corrected versions? But now I see the "corrected" versions are called |
Oh, I see - I misread that reference 😳. I think you're right |
I think the hold up here is that nobody has been able to provide definitive clarity around the types used. Unless the original authors @bicycle1885 or @SabrinaJaye can comment, someone would need to go through the code and rationalise the already made decisions, though I think @timholy has done this and is most familiar with this aspect of the code. So @timholy, I am happy to take your preference, whether to subtype from |
SubstitutionMatrix is not a subtype of AbstractArray. This may or may not be intentional, but currently, the number of supported operations is fairly minimal. This provides a few more conveniences for users who might want to manipulate them beyond mere lookup tables.
SubstitutionMatrix is not a subtype of AbstractArray.
This may or may not be intentional, but currently the number of
supported operations is fairly minimal.
This provides a few more conveniences for users who might want to
manipulate them beyond mere lookup tables.
Types of changes
This PR implements the following changes:
(Please tick any or all of the following that are applicable)
📋 Additional detail
similar
,isassigned
, a meaningfuleltype
, and logical indexing forSubstitutionMatrix
.setindex!
now returnsval
rather than the matrix. This allows assignment operations to be chained and is the standard waysetindex!
should behave. (I would call the old behavior a bug, hence the minor increment rather than breaking release.)☑️ Checklist
docs/src/
.[UNRELEASED]
section of the manually curatedCHANGELOG.md
file for this repository.Most of the items in this checklist don't apply, since these are just "missing" Base methods that I'd expect from anything with
Matrix
in the name.