Skip to content
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

NEW: Adds core beta diversity measures #6

Merged
merged 48 commits into from
Jun 11, 2020
Merged

Conversation

ChrisKeefe
Copy link
Collaborator

@ChrisKeefe ChrisKeefe commented Oct 14, 2019

This PR includes:

  • bray-curtis
  • jaccard
  • weighted and unweighted unifrac
  • relevant citations

In addition, this PR attempts to impose consistent quoting across the package, with user-facing strings double-quoted, and non-user-facing strings, keys, etc single-quoted. See fd0e24e

Copy link
Contributor

@thermokarst thermokarst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ChrisKeefe, this PR is looking pretty good. I was poking around at this this morning and noticed a few things that I wanted to bring to your attention. There is a bit of an inconsistency between the util tests, the decorator utils, and the view functions (the actions). In particular, the inconsistency has to do with the "file-based" biom files. The decorators (and their tests) all are set up to work withstr filepaths, but the view annotations in your actions look like:

def faith_pd(table: BIOMV210Format, phylogeny: NewickFormat) -> pd.Series:

Note, the annotation for table is BIOMV210Format, not str. I think it would make sense to "consolidate" on one view type: BIOMV210Format. You can start to do that by following a few of the suggestions I have provided, inline.

q2_diversity_lib/_util.py Outdated Show resolved Hide resolved
q2_diversity_lib/tests/test_alpha.py Outdated Show resolved Hide resolved
q2_diversity_lib/tests/test_util.py Outdated Show resolved Hide resolved
q2_diversity_lib/tests/test_util.py Outdated Show resolved Hide resolved
q2_diversity_lib/_util.py Outdated Show resolved Hide resolved
@ChrisKeefe

This comment has been minimized.

@thermokarst

This comment has been minimized.

@thermokarst

This comment has been minimized.

@ChrisKeefe

This comment has been minimized.

@ChrisKeefe ChrisKeefe marked this pull request as ready for review May 21, 2020 23:07
@ChrisKeefe ChrisKeefe marked this pull request as draft May 22, 2020 03:06
@thermokarst

This comment has been minimized.

@ChrisKeefe

This comment has been minimized.

@ChrisKeefe

This comment has been minimized.

@thermokarst

This comment has been minimized.

@thermokarst

This comment has been minimized.

@thermokarst

This comment has been minimized.

Copy link
Contributor

@thermokarst thermokarst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a pass through the tests, they look great! I have a few minor requests (see inline). Thanks!

setup.py Show resolved Hide resolved
q2_diversity_lib/tests/data/extra_tip.tree Outdated Show resolved Hide resolved
q2_diversity_lib/tests/data/faith_test.tree Outdated Show resolved Hide resolved
q2_diversity_lib/tests/data/missing_tip.tree Outdated Show resolved Hide resolved
q2_diversity_lib/tests/data/root_only.tree Outdated Show resolved Hide resolved
q2_diversity_lib/tests/data/two_feature_table.tsv Outdated Show resolved Hide resolved
q2_diversity_lib/tests/test_alpha.py Outdated Show resolved Hide resolved
q2_diversity_lib/tests/test_alpha.py Show resolved Hide resolved
@ChrisKeefe ChrisKeefe assigned thermokarst and unassigned ChrisKeefe Jun 10, 2020
@ChrisKeefe ChrisKeefe requested a review from thermokarst June 10, 2020 21:30
@ChrisKeefe ChrisKeefe added the stat:DO-NOT-MERGE Please do not merge this until this label has been removed. label Jun 11, 2020
@ChrisKeefe
Copy link
Collaborator Author

@thermokarst, I'm running into some squirrelly issues (Multiple values for argument thread) that I thought were resolved when I run these methods through q2-diversity, and will have to do some troubleshooting tomorrow to determine the source of the issue. Sorry for any inconvenience.

@thermokarst
Copy link
Contributor

Just a mis-wiring issue - the problem is in qiime2/q2-diversity#273, not in this PR. You just need to match up the right n_jobs vs threads params in the various methods:

applying this patch (below) to commit d2ec98091bc2c8c0ca05ec6c7c928dd5ae021d7b (in PR 273) fixes it up:

diff --git a/q2_diversity/_core_metrics.py b/q2_diversity/_core_metrics.py
index 47a7b89..4f1ab57 100644
--- a/q2_diversity/_core_metrics.py
+++ b/q2_diversity/_core_metrics.py
@@ -61,9 +61,9 @@ def core_metrics_phylogenetic(ctx, table, phylogeny, sampling_depth, metadata,
 
     dms = []
     dms += unweighted_unifrac(table=cr.rarefied_table, phylogeny=phylogeny,
-                              n_jobs=n_jobs)
+                              threads=n_jobs)
     dms += weighted_unifrac(table=cr.rarefied_table, phylogeny=phylogeny,
-                            n_jobs=n_jobs)
+                            threads=n_jobs)
 
     pcoas = []
     for dm in dms:

@thermokarst
Copy link
Contributor

I'm going to merge this PR so that busywork can chew away at it, we'll address any fallout in additional PRs. Then, if everything goes to plan, you'll have a good dev env to work with in travis, which should help keep you lined up on qiime2/q2-diversity#273.

Also, I will do one sweeping pass over the whole repo tomorrow, to do a "holisitic" review.

@thermokarst thermokarst removed the stat:DO-NOT-MERGE Please do not merge this until this label has been removed. label Jun 11, 2020
@thermokarst thermokarst merged commit 2995d07 into qiime2:master Jun 11, 2020
@ChrisKeefe
Copy link
Collaborator Author

Thanks for pushing this through, @thermokarst. Totally possible it is a mis-wiring issue, but it may be elsewhere. The code I'm testing already looks like this:
image

I'll give it a thorough looking over when I'm fresh. :)

@thermokarst
Copy link
Contributor

when I'm fresh

I think that sounds like a great idea.


Let's:

a) stop discussing here (a least for now), and instead move over to qiime2/q2-diversity#273
b) rather than sharing isolated snippets and error messages with no context, please use the tools we have available: push any problematic code up to github, that way we can discuss code & any relevant errors that crop up in the ci unit tests.

Thanks!

@ChrisKeefe
Copy link
Collaborator Author

For posterity, the multiple values bug mentioned above was addressed in #18.

@ChrisKeefe ChrisKeefe deleted the bDiv branch November 30, 2020 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants