NEW: Adds core beta diversity measures

[ChrisKeefe]

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 https://github.com/qiime2/q2-diversity-lib/pull/6/commits/fd0e24e75ef07e348b96d169e371e25e03ed73dd

[ChrisKeefe]

@thermokarst, did you run into any issues triggering your first GH Actions workflow execution when you were setting up q2-phylogenomics?

In the branch I just merged here, Actions took a few minutes and a commit or two before it started running. On this branch, though, it's been two hours with no response. The workflow is in the appropriate folder, actions are enabled for this repo, I've even removed the Travis CI checks to see whether that fixes the issue, but no luck. :shrug:

It's totally possible it's just taking its time, but I figured it was worth asking.

[thermokarst]

I think its because this is a Draft Pull Request, instead of a Pull Request. That means it won't trigger the event listed here:

https://github.com/qiime2/q2-diversity-lib/pull/6/files#diff-4189f083ccfd654feb064e5cef225ddaR4

But, I'm not 100% sure that that is the case.

[thermokarst]

Scratch that - this draft PR (link) is running actions just fine...

[ChrisKeefe]

But I want that to be the problem! I'm going to switch toggle this over to "real PR" status for a minute just to check. :pouting_cat:

[thermokarst]

Action workflows appear to generally work on this repo: https://github.com/qiime2/q2-diversity-lib/pull/13/checks?check_run_id=698503717

Thoughts as to why it isn't working here:

• syntax error in workflow file
• something funky with all the force pushes (?)
• luck?

[ChrisKeefe]

Alright, @thermokarst. I found my workflows over in chriskeefe/q2-diversity-lib, and I suspect they're not showing up in the PR because qiime2/q2-diversity doesn't know about them yet. The UX on this is pretty gross, but totally reasonable given this snippet from the docs:

When you create a pull request from a forked repository to the base repository, GitHub sends the pull_request event to the base repository and no pull request events occur on the forked repository.

Rather than go monkey around in the fork every time I push a commit here, I'm going to open a fresh PR with only the actions, which we can merge to master before this PR wraps up.

[ChrisKeefe]

Well, that PR's open, and happily running GH Actions checks against the code base, just as we expected here.

I'm guessing those checks fired off on the "create PR" action, which doesn't sit well with my theory above. We'll see if it makes any difference to this PR when merged. :man_shrugging:

[thermokarst]

I found my workflows over

Interesting!

not showing up in the PR because qiime2/q2-diversity doesn't know about them yet.

Do you mean qiime2/q2-diversity-lib? I'll assume so. As far as "not knowing about them yet", I don't think that's the case, otherwise why would the GHA workflow run in your latest PR (link)? I've developed a few of of these now and they've all triggered in the PR that introduced them.

The UX on this is pretty gross, but totally reasonable given this snippet from the docs:

I think you might be reading that quote backwards - the forked repo is ChrisKeefe and the base repo is qiime2:

When you create a pull request from a forked (ChrisKeefe) repository to the base (qiime2) repository, GitHub sends the pull_request event to the base (qiime2) repository and no pull request events occur on the forked (ChrisKeefe) repository.

This interpretation that I propose above is consistent with what I have seen on a few of these workflows that I have worked on.

I'm guessing those checks fired off on the "create PR" action

That's an interesting idea, let's take a closer look at the docs:

https://help.github.com/en/actions/reference/events-that-trigger-workflows#pull-request-event-pull_request

My eye got caught on:

Note: By default, a workflow only runs when a pull_request's activity type is opened, synchronize, or reopened. To trigger workflows for more activity types, use the types keyword.

That seems to fall in line with what you propose about the create (opened) event. Although I'm curious what the synchronize event is...

[thermokarst]

BTW, I wonder if closing and reopening this PR would also do the trick, since that would (in theory) trigger the reopened event.

[thermokarst]

Oh, and another thought, and maybe this is where you were going above, and I just misunderstood, but I wonder if the events were going to ChrisKeefe because you had the PR-within-a-PR going on (the PR on your fork, to your fork, to the base repo). That might help explain that.

Oh yeah, and that totally does explain the "synchronize" events, too --- the sync events were triggering just fine, but they were triggering on the original base (ChrisKeefe).

[thermokarst]

Hey look at that, skipping the PR-inception and just pushing commits straight up works as advertised! Okay, I'll revert that commit now.

[thermokarst]

https://github.com/qiime2/q2-diversity-lib/runs/698569373

[thermokarst]

Hey @ChrisKeefe - the build failed because of some issues with the tags - I deleted the 2020.5.0.dev0 tag, and "retagged" 2020.6.0.dev0 on the tip of master. Then I triggered a re-run on GHA and it look like it has succeeded on linux (mac is still cooking a time of writing, but I think we're expecting this to fail).

[ChrisKeefe]

We are indeed expecting this test failure. Thanks for keeping an eye on things!

[thermokarst]

testing testing 1 2 3

[ChrisKeefe]

@ebolyen, @thermokarst, this PR has gotten long enough! :laughing: I'm opening it for review as originally intended (core-metrics measures only), and will open a separate PR to master for "fancy" unifracs.

[thermokarst]

I should have some time next week to look at this - thanks @ChrisKeefe!

[ChrisKeefe]

@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]

Just a mis-wiring issue - the problem is in https://github.com/qiime2/q2-diversity/pull/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]

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 https://github.com/qiime2/q2-diversity/pull/273.

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

[ChrisKeefe]

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:

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

[thermokarst]

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 https://github.com/qiime2/q2-diversity/pull/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]

For posterity, the multiple values bug mentioned above was addressed in https://github.com/qiime2/q2-diversity-lib/pull/18.