-
Notifications
You must be signed in to change notification settings - Fork 65
Evolutionary distance computation for BioSequence. #228
Conversation
Current coverage is 82.20% (diff: 75.86%)
|
macro checkambig(expr) | ||
quote | ||
if isambiguous(a[i]) || isambiguous(b[i]) | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using break
instead of throw
an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be a continue not a break, that's a typo on my part.
@@ -170,7 +232,58 @@ end | |||
|
|||
# K80 Distance computation. | |||
|
|||
<<<<<<< HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! @Ward9250
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen it - it's gone on my uncommitted workings 😆. Code committed here might be messy in places for a while. I'm basing the design on a C design, which uses a lot of repetitive code, and macros. In julia, with generated functions, type hierarchy, and multiple dispatch I think we can do better and be more concise, but I'm still feeling my way to the best way of doing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds awesome! Excuse the uninvited interjections, for some reason the diffs display in email alerts and though I'd let you know in case you missed it! Sorry for the noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not problematic for me at all, in fact we encourage such participation! :)
093cfcd
to
3394d73
Compare
8fbdc36
to
2715c34
Compare
9f2faf2
to
dd6784c
Compare
Kimura80, | ||
distance, | ||
Raw, # alias for N_Mutations{DifferentMutation} | ||
P, # alias for P_Distance{DifferentMutation} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P
and Raw
looks too short and vague.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok it can be removed.
@bicycle1885 Based on your comments I've changed N_Mutations{T} to Count{T}, and P_Distance{T} to Proportion{T}, removing the underscore, and I think, giving them both more semantic names e.g. I think the meaning of e.g. Count{Transitions} is clear, as is Proportion{Transitions}, indeed Proportion is more descriptive for people who have never heard of the measure referred to as a p-distance (Phylogeneticists and population geneticists certainly would have heard of p-distance, but other biologists may not have). |
d76af82
to
8c4fbe8
Compare
b385223
to
cdeb1bc
Compare
# ----- | ||
|
||
# Evolutionary distances | ||
abstract EvolutionaryDistances |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this plural?
immutable Kimura80 <: TsTv end | ||
|
||
typealias JC69 JukesCantor69 | ||
typealias K80 Kimura80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these type aliases are redundant. We should stick to use a single type name to avoid unnecessary confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JC69 and K80 and other models in phylogenetics and evo.genetics have such few letter abbreviations that are commonly used, but they can be defined elsewhere in specific places if really needed, so I took them away.
1 similar comment
@bicycle1885 tests pass now 👍 if there's nothing else I'd like to merge and tick a few things off of #266. |
JukesCantor69, | ||
Kimura80, | ||
JC69, # alias for JukesCantor69 | ||
K80, # alias for Kimura80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these type aliases are already removed (a peculiar fact is that Julia doesn't warn you for this kind of mistakes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed odd that the build didn't flag that up! :O
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok just did a git commit amend, that should be gone now.
f9d0ecd
to
6d7980d
Compare
Okay, please go ahead . |
6d7980d
to
3c1a893
Compare
Fulfills some of issue #266
This PR will start with Raw mutations counts, P distances, JC69, and K80. For BioSequences.