-
Notifications
You must be signed in to change notification settings - Fork 17
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
G unifrac #19
G unifrac #19
Conversation
@wasade Can you update this branch with the latest version of master? |
BAM BAM |
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.
Looks good, just a couple questions and suggestions.
q2_state_unifrac/plugin_setup.py
Outdated
'the table.') | ||
}, | ||
outputs=[('distance_matrix', DistanceMatrix % Properties('phylogenetic'))], | ||
name='Wweighted normalized UniFrac', |
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.
Wweighted -> Weighted 🏋️ 🏋️♀️
description=('This method computes weighted unnormalized UniFrac') | ||
description=('This method computes weighted unnormalized UniFrac as ' | ||
'described in Lozupone et al. 2007 Appl Environ Microbiol; ' | ||
'DOI: 10.1128/AEM.01996-06') |
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.
Would be nice to add the citation_text
for all these papers, see for example:
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.
Thanks! Isn't that at the plugin level though, and not at the method level? These citations are method specific
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.
You are exactly right!
sucpp/test_su.cpp
Outdated
w_task_p.g_unifrac_alpha = 1.0; | ||
su::unifrac(table, tree, su::generalized, w_strides, w_strides_total, &w_task_p); | ||
|
||
// as computed by GUniFrac |
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.
For sanity in the future, might be a good idea to include package version that produced this output.
sucpp/test_su.cpp
Outdated
d0_task_p.g_unifrac_alpha = 0.0; | ||
su::unifrac(table, tree, su::generalized, d0_strides, d0_strides_total, &d0_task_p); | ||
|
||
// as computed by GUniFrac |
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.
Same as above.
const su::task_parameters* task_p) { | ||
|
||
if(table.n_samples != task_p->n_samples) { | ||
fprintf(stderr, "Task and table n_samples not equal\n"); |
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.
What does this mean?
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.
table
is not passed into the kernels, so table->n_samples
is not accessible without a lot of monkeying. This is just a sanity check that the task parameters reflect the table (loosely)
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.
👍
sucpp/unifrac.hpp
Outdated
unsigned int end, | ||
unsigned int tid); | ||
const task_parameters* task_p); | ||
//void unweighted_unifrac(biom &table, |
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.
Can you remove these lines?
Depends on #14 and #18, merge those first
We currently validate
a=1.0
. TheGUniFrac
R package does not include tests unfortunately. Before merge, it would be good to include verification ofa=0.0
anda=0.5
. Note thata=0.0
is unweighted UniFrac only if the proportions are dichotomized.