Adds a method to use a high performance implementation of UniFrac #143

ElDeveloper · 2017-09-20T06:17:14Z

This PR adds the necessary code to use the implementation of UniFrac in https://github.com/biocore/unifrac . This method is recommended for large datasets.

As a comparison, for a table with ~120 samples and a tree with 20K tips running time is: 2 minutes and 27 seconds with the current implementation, and 2 seconds with the new implementation. @wasade has been benchmarking this more thoroughly but, thought this served as a nice and small comparison.

As discussed at the developer's meeting, this method does not replace beta_phylogenetic, as it is still fairly new, instead it is provided as an alternative (beta_phylogenetic_hpc) that users can test out.

Currently, there's a few things missing in this PR:

Tests, it would make more sense to add a few files to the repo, rather than having to create those on the fly, but I am not sure if there's a preferred way to do this.
Deployment, this depends on https://github.com/biocore/unifrac which does not yet have a conda recipe (@wasade is working on this).
Citation information, it would be nice to include more information about the specific papers that should be cited for each metric. We previously had this when this was its own plugin, see here.
Input validation, there's a few validations that are currently not being performed as part of https://github.com/biocore/unifrac but that are made here or in skbio, for example see (Small trees/tables fail biocore/unifrac#37, Trees with less than 4 tips return nan biocore/unifrac#36, Validate input tree biocore/unifrac#35, Validate input table biocore/unifrac#34 and Should UniFrac ever return NaN? biocore/unifrac#33 )

This implementation is suggested for large datasets.

wasade · 2017-09-20T14:48:42Z

Thanks, @ElDeveloper! One additional point, we include support for Generalized UniFrac and Variance Adjusted UniFrac. We also support meta UniFrac but it assumes variadic arguments support (qiime2/qiime2#234); it may be nice to put in a near term work around though.

jairideout · 2017-09-20T15:57:49Z

Tests, it would make more sense to add a few files to the repo, rather than having to create those on the fly, but I am not sure if there's a preferred way to do this.

Take a look at some of the other QIIME 2 repos for examples of adding test data files -- we have a custom unittest.TestCase subclass that provides a helper for locating test data. I think q2-types is a good example -- let us know if you run into issues. Please try to keep the files small.

Citation information, it would be nice to include more information about the specific papers that should be cited for each metric. We previously had this when this was its own plugin, see here.

Citation support is currently limited to free text, so you can add whatever citations you want to the plugin using the citation_text field (I don't recommend putting citation info in the description field like you have in your linked example).

ElDeveloper · 2017-09-20T17:21:05Z

Thanks for the guidance Jai, I'll put these in place.

…

On (Sep-20-17|15:57), Jai Ram Rideout wrote: > Tests, it would make more sense to add a few files to the repo, rather than having to create those on the fly, but I am not sure if there's a preferred way to do this. Take a look at some of the other QIIME 2 repos for examples of adding test data files -- we have a custom `unittest.TestCase` subclass that provides a helper for locating test data. I think q2-types is a good example -- let us know if you run into issues. Please try to keep the files small. > Citation information, it would be nice to include more information about the specific papers that should be cited for each metric. We previously had this when this was its own plugin, see here. Citation support is currently limited to free text, so you can add whatever citations you want to the plugin using the `citation_text` field (I don't recommend putting citation info in the `description` field like you have in your linked example). -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #143 (comment)

ElDeveloper · 2017-09-20T22:40:21Z

@jairideout, I've added the citation text and tests. 👍 This should be contingent on @wasade publishing a conda package, but is otherwise good to review.

wasade · 2017-09-20T22:54:34Z

@ElDeveloper, would it be possible to expose bypass_tips, a.k.a. fast mode? As a parameter description: "Compute will be reduced by ~50% with this option yielding an approximate result. The option disregards computing UniFrac at the tips of the phylogeny and instead only computes it on the internal nodes. In a bifurcating tree, the tips correspond to approximately 50% of the vertices in the tree, which is how the compute time reduction is achieved. The use of this parameter is analogous in concept to switching from 99% OTUs to 97% OTUs."

@mortonjt

Thanks @mortonjt @wasade! Add bypass-tips

ElDeveloper · 2017-09-21T23:59:17Z

Tests are passing 🎉

ebolyen · 2017-09-23T18:48:40Z

Looks like it worked! We've got new environment files to test with 🎉!

wasade · 2017-09-23T19:45:52Z

That's excellent!! On Sep 23, 2017 11:49, "Evan Bolyen" <[email protected]> wrote: Looks like it worked <qiime2/environment-files@2e173e3>! We've got new environment files to test with 🎉! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#143 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAc8sg2QRWYO67iYFhbdcmJF_37LwJcYks5slVKIgaJpZM4PdZF2> .

ebolyen

Sorry for the late review, this is generally looking really good though! I'm going to pull this down and play with it a bit, but here's a few comments in the meanwhile.

ebolyen · 2017-09-25T22:47:00Z

q2_diversity/_beta/_method.py

@@ -63,6 +69,27 @@ def beta_phylogenetic(table: biom.Table, phylogeny: skbio.TreeNode,
    return results


+def beta_phylogenetic_hpc(table: BIOMV210Format, phylogeny: NewickFormat,


How you guys feel about something like: beta_phylogenetic_alt, then it may not be too much of a surprise if/when this gets merged into beta_phylogenetic.

We could also do something with unifrac like unifrac_variants or something, but given you are trying to roughly match the signature, it seems like an -alt suffix might work pretty well.

Great suggestion, changed to alt!

ebolyen · 2017-09-25T22:49:13Z

q2_diversity/plugin_setup.py

+                   'https://doi.org/10.1186/1471-2105-12-118.'
+                   'Generalized UniFrac: '
+                   'Chen et al. 2012 Bioinformatics; DOI: '
+                   '10.1093/bioinformatics/bts342')


I don't think we have a convention here, but some newline characters might work well to separate these

I couldn't see any changes with the new lines I added. ¯_(ツ)_/¯

ebolyen · 2017-09-25T22:49:51Z

q2_diversity/plugin_setup.py

+                                         'weighted_unnormalized_unifrac',
+                                         'weighted_normalized_unifrac',
+                                         'generalized_unifrac']),
+                'n_jobs': Int,


Should this be Int % Range(1, None)? What happens with negatives or zero?

Negative numbers are not supported, so this is great!

ebolyen · 2017-09-25T22:50:47Z

q2_diversity/plugin_setup.py

+                                         'generalized_unifrac']),
+                'n_jobs': Int,
+                'variance_adjusted': Bool,
+                'alpha': Float, 'bypass_tips': Bool},


add a newline so we can find bypass_tips in the future

Not sure what this means, but done.

ebolyen · 2017-09-25T22:52:45Z

q2_diversity/plugin_setup.py

+        'metric': 'The beta diversity metric to be computed.',
+        'n_jobs': 'The number of workers to use.',
+        'variance_adjusted': ('Perform variance adjustment based on Chang et '
+                              'al. BMC Bioinformatics 2011'),


Is it possible to briefly describe what that means?

I've added an explanation, let me know if it makes sense, also CCing @wasade to keep him in the loop.

ebolyen · 2017-09-25T22:57:29Z

q2_diversity/plugin_setup.py

+                        'correspond to approximately 50% of the vertices in '
+                        'the tree, which is how the compute time reduction is'
+                        ' achieved. The use of this parameter is analogous '
+                        'in concept to switching from 99% OTUs to 97% OTUs.')


Maybe just say:

In a bifurcating tree, the tips make up about 50% of the nodes in a tree. By ignoring them, specificity can be traded for reduced compute time.

Or something like that.

That's much more concise, thanks.

ebolyen · 2017-09-25T22:58:41Z

q2_diversity/plugin_setup.py

+                                         'generalized_unifrac']),
+                'n_jobs': Int,
+                'variance_adjusted': Bool,
+                'alpha': Float, 'bypass_tips': Bool},


Float -> Float % Range(0, 1, inclusive_end=True)

Great, I had previously tried Float % Range(0, 1) but that wasn't working, I'm glad this works.

ebolyen · 2017-09-25T23:00:30Z

q2_diversity/plugin_setup.py

+                 "2012), Variance Adjusted UniFrac (Chang et al. 2011), "
+                 "as well as Weighted normalized and unnormalized UniFrac "
+                 "(Lozupone et al. 2007), unweighted UniFrac "
+                 "(Lozupone et al. 2005), and metagenomic UniFrac "


I don't think metagenomic UniFrac is exposed at the moment.

Good catch.

ebolyen · 2017-09-25T23:02:42Z

q2_diversity/_beta/_method.py

+    elif metric == 'weighted_normalized_unifrac':
+        f = unifrac.weighted_normalized
+    elif metric == 'generalized_unifrac':
+        f = partial(unifrac.generalized, alpha=alpha)


This is fine, but a dictionary lookup might be cleaner (that's usually what we do).

It made things cleaner, though we needed a bit of special casing though 👍

ebolyen · 2017-09-25T23:04:34Z

q2_diversity/_beta/_method.py

+def beta_phylogenetic_hpc(table: BIOMV210Format, phylogeny: NewickFormat,
+                          metric: str, n_jobs: int=1,
+                          variance_adjusted: bool=False,
+                          alpha=1.0,


I don't have a strong recommendation, but it might be nice to error if alpha is passed when the metric isn't generalized UniFrac. That would require you set the default to something like None though, which definitely obfuscates things a bit.

I followed your suggestion here.

wasade · 2017-09-26T17:47:36Z

q2_diversity/_beta/_method.py

@@ -63,6 +76,31 @@ def beta_phylogenetic(table: biom.Table, phylogeny: skbio.TreeNode,
    return results


+def beta_phylogenetic_alt(table: BIOMV210Format, phylogeny: NewickFormat,


Would it be possible to politely catch and err if the table has < 4 samples? That's an edge case we haven't handled yet behind the scenes

I am not sure that this is the reason why we've observed failures, for all the unit tests we compute distances on a 2x2 sample matrix, and those tests pass.

wasade · 2017-09-26T17:49:18Z

q2_diversity/plugin_setup.py

 )

 plugin.methods.register_function(
    function=q2_diversity.beta_phylogenetic,
    inputs={'table': FeatureTable[Frequency],
            'phylogeny': Phylogeny[Rooted]},
    parameters={'metric': Str % Choices(beta.phylogenetic_metrics()),
-                'n_jobs': Int},
+                'n_jobs': Int % Range(1, None)},


This is minor, and ultimately whatever fits better into the surrounding terminology is preferred, but it is more correct to describe this as threads.

Jobs is probably better for now, since it sounds like this is actually CPU threads, and not a process thread.

It's process threads (i.e., pthread) that we bind to cores to reduce thread hopping and to leverage NUMA's default local allocation policy.

When I wrote this, I knew it was misleading, but carried on since it matches beta_phylogenetic. Would probably be nice to have a standard designation for "workers" all throughout QIIME.

wasade · 2017-09-26T17:50:33Z

q2_diversity/plugin_setup.py

+        'n_jobs': 'The number of workers to use.',
+        'variance_adjusted': ('Perform variance adjustment based on Chang et '
+                              'al. BMC Bioinformatics 2011. Weights distances '
+                              'by the number of tips evaluated for each '


It weights based on the proportion of the relative abundance represented between the samples at a given node under evaluation

wasade · 2017-09-26T17:51:55Z

q2_diversity/plugin_setup.py

+                  ' proportions are dichotomized.'),
+        'bypass_tips': ('In a bifurcating tree, the tips make up about 50% of '
+                        'the nodes in a tree. By ignoring them, specificity '
+                        'can be traded for reduced compute time.')


Perhaps add: "time. This has the effect of collapsing the phylogeny, and is analogous in concept to moving from 99% to 97% OTUs"?

ebolyen

Two more comments

ebolyen · 2017-09-26T18:58:03Z

q2_diversity/_beta/_method.py

+
+    if alpha is not None and metric != 'generalized_unifrac':
+        raise ValueError('The alpha parameter is only allowed when the choice'
+                         ' of metric is generalized_unifrac')


The "opposite" of this causes unifrac to raise an error, but the user is too far removed to be able to pinpoint the cause.

So we should add a branch to either set alpha=1 when alpha == None or to explain that alpha needs to be set. Either should work.

Great catch!!! 👏

ebolyen · 2017-09-26T18:59:50Z

q2_diversity/_beta/_method.py

+        f = metrics[metric]
+
+    # unifrac processes tables and trees should be filenames
+    return f(str(table), str(phylogeny), threads=n_jobs,


when n_jobs is greater than logical cores, we get a segfault (when we're lucky). We should just guard that value to be at or less than cores present on the host.

psutil to the rescure 💪

ebolyen · 2017-09-26T22:25:02Z

ci/recipe/meta.yaml

@@ -32,6 +32,7 @@ requirements:
    - ipywidgets
    - scikit-bio
    - scikit-learn
+    - psutilt


ElDeveloper · 2017-09-26T22:39:38Z

Thanks for fixing @ebolyen!

wasade · 2017-09-26T22:40:48Z

Woooo!!! Thank you guys! On Sep 26, 2017 3:39 PM, "Evan Bolyen" <[email protected]> wrote: Merged #143 <#143>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#143 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAc8shUpc72VISL-B-_gPmRj1HTELnY9ks5smX0JgaJpZM4PdZF2> .

ElDeveloper added 2 commits September 19, 2017 22:23

ENH: Adds method to use strided state Unifrac

d1c6b75

This implementation is suggested for large datasets.

ENH: Check for available metrics

ea52be1

jairideout added the enhancement label Sep 20, 2017

ElDeveloper added 3 commits September 20, 2017 15:21

TST: Add unit tests

adf0248

TST: Add test data

dfc78f9

DOC: Add citation text

fb0b28b

BLD: Add unifrac

e4aa0ef

Thanks @mortonjt @wasade! Add bypass-tips

ElDeveloper force-pushed the state-unifrac branch from 62a5602 to e4aa0ef Compare September 21, 2017 21:56

ElDeveloper added 2 commits September 21, 2017 15:34

BLD: Conda forge :anvil:

e246e02

BLD: 1.8.18

5f06c76

ElDeveloper force-pushed the state-unifrac branch from 0127155 to 5f06c76 Compare September 21, 2017 23:12

STY: 💄 🤙🏽

8200a7d

ElDeveloper changed the title ~~Adds a method to use a high performance implementation of UniFrac [WIP]~~ Adds a method to use a high performance implementation of UniFrac Sep 21, 2017

ElDeveloper added 4 commits September 22, 2017 12:03

BLD: Pin down HDF5 version 🔨

d624c59

BLD: Fix recipe syntax 🐍

17758c1

BLD: Remove recipe mods

be2a581

BLD: Shouldn't be a problem after the CI updates

80091a3

ElDeveloper force-pushed the state-unifrac branch from 64dc728 to 80091a3 Compare September 22, 2017 21:36

thermokarst requested a review from ebolyen September 25, 2017 17:52

thermokarst assigned ebolyen Sep 25, 2017

ebolyen requested changes Sep 25, 2017

View reviewed changes

Address review changes by @ebolyen

79e0a29

STY: PEP🎱

bf7ffef

wasade reviewed Sep 26, 2017

View reviewed changes

ebolyen requested changes Sep 26, 2017

View reviewed changes

ElDeveloper added 3 commits September 26, 2017 14:13

DOC/TST/ENH/BUG: Changes addressing reviews

0eaba83

Merge branch 'master' into state-unifrac

df4fbc1

STY: PEP🎱

ae91c7d

ebolyen requested changes Sep 26, 2017

View reviewed changes

ci/recipe/meta.yaml Outdated

@@ -32,6 +32,7 @@ requirements:

- ipywidgets

- scikit-bio

- scikit-learn

- psutilt

Copy link

Member

ebolyen Sep 26, 2017

Choose a reason for hiding this comment

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

extra t

SQUASH: fix psutil

8b88643

ebolyen approved these changes Sep 26, 2017

View reviewed changes

ebolyen merged commit dd1322b into qiime2:master Sep 26, 2017

ebolyen mentioned this pull request Sep 26, 2017

BLD: adds missing package data #148

Merged

		@@ -63,6 +69,27 @@ def beta_phylogenetic(table: biom.Table, phylogeny: skbio.TreeNode,
		return results


		def beta_phylogenetic_hpc(table: BIOMV210Format, phylogeny: NewickFormat,

		@@ -63,6 +76,31 @@ def beta_phylogenetic(table: biom.Table, phylogeny: skbio.TreeNode,
		return results


		def beta_phylogenetic_alt(table: BIOMV210Format, phylogeny: NewickFormat,

Adds a method to use a high performance implementation of UniFrac #143

Adds a method to use a high performance implementation of UniFrac #143

Conversation

ElDeveloper commented Sep 20, 2017 • edited Loading

wasade commented Sep 20, 2017

jairideout commented Sep 20, 2017

ElDeveloper commented Sep 20, 2017 via email

ElDeveloper commented Sep 20, 2017

wasade commented Sep 20, 2017

ElDeveloper commented Sep 21, 2017

ebolyen commented Sep 23, 2017

wasade commented Sep 23, 2017 via email

ebolyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebolyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElDeveloper commented Sep 26, 2017

wasade commented Sep 26, 2017 via email

ElDeveloper commented Sep 20, 2017 •

edited

Loading