Fixes Qiita's 2237 issue #18

josenavas · 2017-10-22T19:20:02Z

Just want to make sure that tests are executing correctly here

josenavas · 2017-10-23T04:51:08Z

This is ready for review. State Unifrac is failing to install and I don't know why.

wasade · 2017-10-23T15:21:13Z

.travis.yml

+  # Installing state unifrac
+  - conda install --yes -c conda-forge cython
+  - conda install --yes -c biocore unifrac
+  # - sed -i 's/^CXXBASE=.*/CXXBASE=clang++/' `which h5c++`


remove please

wasade · 2017-10-23T15:22:01Z

qp_qiime2/__init__.py

-    'i-table': ('artifact', ['BIOM']),
-    'p-sampling-depth': ['integer', 1000]
+    'BIOM table': ('artifact', ['BIOM']),
+    'Sampling depth': ['integer', 1000]


is 1000 the default? it looks like this is a replicated value with the dflt_param_set? why 1000?

Yes, this is a default per parameter while the dflt_param_set is a set of defaults for all parameters. In this case, this command only has one parameter and that's why it looks replicated. It is also possible, however, to add more than one set of dflt_param_set in which we change this value.

wasade · 2017-10-23T15:22:18Z

qp_qiime2/__init__.py

-        'jaccard'],
-    'i-tree': ['choice:["default", "None"]', 'None']}
+    'Diversity metric': [
+        'choice:%s' % dumps(list(BETA_DIVERSITY_METRICS.keys())),


no need for .keys()

wasade · 2017-10-23T15:22:48Z

qp_qiime2/__init__.py

-    'i-tree': ['choice:["default", "None"]', 'None']}
+    'Diversity metric': [
+        'choice:%s' % dumps(list(BETA_DIVERSITY_METRICS.keys())),
+        'Jaccard similarity index'],


unifrac metrics + jaccard only? what about bray curtis or others?

This is the default value, and we can only choose one. The way it is done in Qiita is that each command executes only 1 metric, because we don't support an arbitrary number of outputs. Since by default we don't have a tree, we've chosen a non-phylogenetic metric as default. The user can, however, change this value on the interface.

wasade · 2017-10-23T15:23:07Z

qp_qiime2/__init__.py

+    'Diversity metric': [
+        'choice:%s' % dumps(list(BETA_DIVERSITY_METRICS.keys())),
+        'Jaccard similarity index'],
+    'Phylogenetic tree': ['choice:["default", "None"]', 'None']}


it's possible to perform a phylogenetic metric without a tree?

wasade · 2017-10-23T15:37:14Z

qp_qiime2/tests/test_qiime2.py

@@ -41,9 +41,10 @@ def tearDown(self):
                    remove(fp)

    def test_rarefy(self):
-        params = {'p-sampling-depth': 2, 'i-table': 5}
+        params = {'Sampling depth': 2, 'BIOM table': 5}


5 doesn't look like a biom table or path to a biom table. Would it be more correct to describe this as "BIOM table artifact ID"?

I think from the point of view of the interface this makes sense - we agreed that we don't necessarily want to expose the word Artifacts to the user interface to avoid confusion.

I think the blending of UI and compute logic is confusing personally.

IMO, the way I see this, is that the plugin is doing the translation between the compute logic and the UI. At the end, the actual logic is in Q2 not here...

It seems like the responsibilities of this plugin are to:

perform computational work directly using the qiime2 API

interact with the qiita database for parameters, artifacts, etc

ensure the above fulfills requirements for a singular and logically distinct user interface

Is that accurate?

Close but not exactly.
The plugin is the interface between Qiita and the underlying tool (in this case Qiime2).

It is not using the Qiime2 API, but the Qiime2 CLI (@antgonza may have more information about this decision).

It doesn't interact with the Database directly, it uses the Qiita REST api to retrieve the different values that it needs.

Since Qiita doesn't know anything about the plugin, the plugin itself needs to make sure to tell Qiita how he wants the different parts of the plugin to be shown to the user.

wasade · 2017-10-23T15:38:08Z

qp_qiime2/tests/test_qiime2.py

-        params['p-metric'] = 'unweighted UniFrac'
-        params['i-tree'] = join(
+        params['Diversity metric'] = 'Unweighted UniFrac'
+        params['Phylogenetic tree'] = join(


This feels inconsistent. The tree is a file path, but the BIOM table is an internal ID?

BIOM is an artifact ID, trees are not stored as artifacts.

The inconsistency on this is confusing but I guess it is what it is.

wasade · 2017-10-23T15:39:43Z

qp_qiime2/tests/test_qiime2.py

-            'i-table': aid, 'p-metric': 'euclidean',
-            'i-tree': 'None'}
+            'BIOM table': aid, 'Diversity metric': 'Euclidean distance',
+            'Phylogenetic tree': 'None'}


Is there an enforcement that absolutely ensures a phylogenetic metric will error appropriately if "Phylogenetic tree" is "None", and similarly, will it error appropriately if a tree is specified with a non-phylogenetic metric? I'm not seeing these tests but perhaps I'm missing them, would it be possible to add if they're not already included?

wasade · 2017-10-23T15:42:21Z

qp_qiime2/tests/test_qiime2.py

@@ -328,7 +342,7 @@ def test_alpha(self):

        # To avoid having to set up all these files, we are gonna test
        # that if phylogenetic and no tree it fails
-        params['i-tree'] = None
+        params['Phylogenetic tree'] = None


I'm confused, above this is set as "None" but now its set as None?

Changed - given how the code works it was behaving correctly anyways.

wasade · 2017-10-23T15:42:49Z

qp_qiime2/tests/test_qiime2.py

+            'Minimum feature frequency across samples': '5',
+            'Maximum feature frequency across samples': '10',
+            'Minimum features per sample': '5',
+            'Maximum features per sample': '9223372036854775807',


Same here, these were int above but now are str?

Good catch - the code forces a cast anyways so it doesn't really matter.

coveralls · 2017-10-23T16:03:11Z

Coverage decreased (-0.2%) to 92.063% when pulling 04002a4 on josenavas:fix-2237 into ab4f1b7 on qiita-spots:master.

josenavas

Thanks @wasade !

josenavas · 2017-10-23T15:49:26Z

qp_qiime2/__init__.py

-    'i-table': ('artifact', ['BIOM']),
-    'p-sampling-depth': ['integer', 1000]
+    'BIOM table': ('artifact', ['BIOM']),
+    'Sampling depth': ['integer', 1000]


Yes, this is a default per parameter while the dflt_param_set is a set of defaults for all parameters. In this case, this command only has one parameter and that's why it looks replicated. It is also possible, however, to add more than one set of dflt_param_set in which we change this value.

josenavas · 2017-10-23T15:49:45Z

qp_qiime2/__init__.py

-        'jaccard'],
-    'i-tree': ['choice:["default", "None"]', 'None']}
+    'Diversity metric': [
+        'choice:%s' % dumps(list(BETA_DIVERSITY_METRICS.keys())),


josenavas · 2017-10-23T15:51:04Z

qp_qiime2/__init__.py

-    'i-tree': ['choice:["default", "None"]', 'None']}
+    'Diversity metric': [
+        'choice:%s' % dumps(list(BETA_DIVERSITY_METRICS.keys())),
+        'Jaccard similarity index'],


This is the default value, and we can only choose one. The way it is done in Qiita is that each command executes only 1 metric, because we don't support an arbitrary number of outputs. Since by default we don't have a tree, we've chosen a non-phylogenetic metric as default. The user can, however, change this value on the interface.

josenavas · 2017-10-23T15:51:48Z

qp_qiime2/__init__.py

 opt_params = {}
 outputs = {'o-pcoa': 'ordination_results'}
 dflt_param_set = {
    'Defaults': {}
 }
 qiime_cmd = QiitaCommand(
-    "pcoa", "Principal Coordinate Analysis",
+    "Generate principal coordinates analysis (PCoA)",


Sure - Done!

josenavas · 2017-10-23T15:53:10Z

qp_qiime2/__init__.py

 outputs = {'q2_visualization': 'q2_visualization'}
 dflt_param_set = {
    'Defaults': {
-        'p-method': 'spearman',
-        'p-permutations': 999}
+        'Correlation method': 'spearman',


josenavas · 2017-10-23T15:57:29Z

qp_qiime2/qiime2.py

-    p_min_features = int(parameters['p-min-features'])
-    p_where = parameters['p-where']
+    artifact_id = int(parameters['BIOM table'])
+    p_max_frequency = int(


They're passes as JSON - but Qiita stores them always as a string because that is how they're reported from the interface.

josenavas · 2017-10-23T15:58:31Z

qp_qiime2/tests/test_qiime2.py

@@ -41,9 +41,10 @@ def tearDown(self):
                    remove(fp)

    def test_rarefy(self):
-        params = {'p-sampling-depth': 2, 'i-table': 5}
+        params = {'Sampling depth': 2, 'BIOM table': 5}


I think from the point of view of the interface this makes sense - we agreed that we don't necessarily want to expose the word Artifacts to the user interface to avoid confusion.

josenavas · 2017-10-23T15:58:50Z

qp_qiime2/tests/test_qiime2.py

-        params['p-metric'] = 'unweighted UniFrac'
-        params['i-tree'] = join(
+        params['Diversity metric'] = 'Unweighted UniFrac'
+        params['Phylogenetic tree'] = join(


BIOM is an artifact ID, trees are not stored as artifacts.

josenavas · 2017-10-23T16:02:43Z

qp_qiime2/tests/test_qiime2.py

@@ -328,7 +342,7 @@ def test_alpha(self):

        # To avoid having to set up all these files, we are gonna test
        # that if phylogenetic and no tree it fails
-        params['i-tree'] = None
+        params['Phylogenetic tree'] = None


Changed - given how the code works it was behaving correctly anyways.

josenavas · 2017-10-23T16:05:30Z

qp_qiime2/tests/test_qiime2.py

+            'Minimum feature frequency across samples': '5',
+            'Maximum feature frequency across samples': '10',
+            'Minimum features per sample': '5',
+            'Maximum features per sample': '9223372036854775807',


Good catch - the code forces a cast anyways so it doesn't really matter.

coveralls · 2017-10-23T16:23:12Z

Coverage decreased (-0.2%) to 92.063% when pulling aa42544 on josenavas:fix-2237 into ab4f1b7 on qiita-spots:master.

josenavas · 2017-10-23T21:06:48Z

@wasade @antgonza any other comments here?

tanaes

Just a couple questions

tanaes · 2017-10-23T21:02:29Z

qp_qiime2/__init__.py

+    'Number of jobs': ['integer', 1],
+    'Adjust variance (phylogenetic only)': ['boolean', False],
+    'Alpha value (Generalized Unifrac only)': ['float', 0],
+    'Bypass tips (phylogenetic only)': ['boolean', False]}
 outputs = {'distance_matrix': 'distance_matrix'}


You've renamed the input dictionary keys (e.g. i-tree to Phylogenetic tree) to be more human-friendly -- should that also be the case for the output dict keys?

...see also lines 73, 91, 110, 127, 138, 157, 174, and 192

@antgonza @adswafford what do you think?

tanaes · 2017-10-23T21:07:10Z

qp_qiime2/__init__.py

-    'p-where': ('string', '')}
+    'Minimum feature frequency across samples': ('integer', 1),
+    'Maximum feature frequency across samples':
+        ('integer', 9223372036854775807),


Is there a more polite / less typo-able way to represent max 64 bit signed integer value?

@antgonza you used this value. Is there any specific reason you used that value instead of using sys.maxint?

Thanks for confirming - removed the hardcoded value

tanaes · 2017-10-23T21:15:37Z

qp_qiime2/tests/test_qiime2.py

+        q2_metrics = beta_methods.union(beta_alt_methods)
+        qp_metrics = set(BETA_DIVERSITY_METRICS.values()).union(
+            STATE_UNIFRAC_METRICS.values()).difference(STATE_UNIFRAC_METRICS)
+        self.assertEqual(q2_metrics, qp_metrics)


So if I parse the logic and the above conversation correctly,

we are hardcoding the dictionaries like BETA_DIVERSITY_METRICS so that we have human-friendly labels and will need to update them, but

this test will check in with the installed Qiime2 version and complain to us if they've added or removed one of the option?

That's correct! The test will fail if there is a metric present in the plugin not present in Qiime 2 and viceversa.

This makes sense to me, although it sure would be nice if the human-readable element of the pair could be totally imported from Qiime2!

Yeah... I think one of the main drawbacks is that the API itself is mainly tailored for programmers and CLI, which have different limitations than a GUI, which is probably why it is harder to provide those human readable elements from Q2 directly

coveralls · 2017-10-23T21:47:46Z

Coverage decreased (-0.02%) to 92.278% when pulling 2f59226 on josenavas:fix-2237 into ab4f1b7 on qiita-spots:master.

josenavas · 2017-10-23T21:56:11Z

I've decided to change the output names after @tanaes comment since it makes sense and I think it will improve the GUI and information to the user.

tanaes · 2017-10-23T21:59:30Z

Awesome, that's much more intelligible for me! Thanks!

josenavas · 2017-10-23T22:01:00Z

🍻

coveralls · 2017-10-23T22:12:35Z

Coverage decreased (-0.02%) to 92.278% when pulling 6a6646e on josenavas:fix-2237 into ab4f1b7 on qiita-spots:master.

antgonza

1 more comment.

antgonza · 2017-10-23T22:13:27Z

qp_qiime2/__init__.py

 dflt_param_set = {
    'Defaults': {}
 }
 qiime_cmd = QiitaCommand(
-    "pcoa", "Principal Coordinate Analysis",
+    "Perform Principal Coordinates Analysis (PCoA)",


This is the only one that is Perform vs. Calculate and the only one that has capital initial letters, fine if we want to leave as this.

That was changed as request by @wasade to match the capitalization of PCoA

coveralls · 2017-10-23T22:33:03Z

Coverage increased (+0.0005%) to 92.298% when pulling fbd3bff on josenavas:fix-2237 into ab4f1b7 on qiita-spots:master.

josenavas added 15 commits October 22, 2017 10:45

Changing command names

bda830b

Fixing tests

bf49bc7

Fixing travis installation

271c773

Fixing installation

cff2a54

Copying the installation from the unifrac repo

ce2c259

Modifying alpha diversity paraemeters

b762257

Changing alpha metrics names

6fa928c

Modifying beta diversity parameters

b4d4d42

Modifying summarize taxa command

0e51f97

Changing filter samples

570574d

Fixing alpha correlation

cf0ba9b

Fixing beta correlation

fa89d85

Fixing beta group significance

568c0d6

Fixing emperor

802e5bb

Fixing all commands

85a42bb

josenavas changed the title ~~[WIP] Fixes Qiita's 2237 issue~~ Fixes Qiita's 2237 issue Oct 23, 2017

josenavas mentioned this pull request Oct 23, 2017

Fix Qiita's 2313 #19

Merged

josenavas added 3 commits October 23, 2017 06:58

Following @antgonza's recommendation

09f8735

Adding --yes

16ddb3a

Fixing Unifrac and upgrading to 2017.9

04002a4

wasade requested changes Oct 23, 2017

View reviewed changes

Addressing @wasade's comments

aa42544

josenavas commented Oct 23, 2017

View reviewed changes

Checking that all metrics are available

2f59226

tanaes approved these changes Oct 23, 2017

View reviewed changes

Modifying output names

6a6646e

Changing hardcoded number by sys.maxsize

fbd3bff

antgonza reviewed Oct 23, 2017

View reviewed changes

antgonza approved these changes Oct 23, 2017

View reviewed changes

wasade approved these changes Oct 23, 2017

View reviewed changes

antgonza merged commit 6b47cf6 into qiita-spots:master Oct 23, 2017

josenavas mentioned this pull request Oct 24, 2017

Fix capitalization/wording in analysis plugin qiita-spots/qiita#2237

Closed

Fixes Qiita's 2237 issue #18

Fixes Qiita's 2237 issue #18

Conversation

josenavas commented Oct 22, 2017

josenavas commented Oct 23, 2017

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

coveralls commented Oct 23, 2017

josenavas 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

coveralls commented Oct 23, 2017

josenavas commented Oct 23, 2017

tanaes 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

coveralls commented Oct 23, 2017

josenavas commented Oct 23, 2017

tanaes commented Oct 23, 2017

josenavas commented Oct 23, 2017

coveralls commented Oct 23, 2017

antgonza 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

coveralls commented Oct 23, 2017