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

Summarize #276 #285

Merged
merged 32 commits into from
Sep 26, 2023
Merged

Summarize #276 #285

merged 32 commits into from
Sep 26, 2023

Conversation

Clockwork-Rat
Copy link
Contributor

Update to reflect new additions in #276

@Clockwork-Rat
Copy link
Contributor Author

@Oddant1 Everything should be good here, it's been tested

q2_feature_table/tests/test_summarize.py Outdated Show resolved Hide resolved
q2_feature_table/tests/test_summarize.py Show resolved Hide resolved
q2_feature_table/tests/test_summarize.py Outdated Show resolved Hide resolved
q2_feature_table/_summarize/_visualizer.py Outdated Show resolved Hide resolved
q2_feature_table/plugin_setup.py Outdated Show resolved Hide resolved
q2_feature_table/_summarize/_visualizer.py Outdated Show resolved Hide resolved
q2_feature_table/tests/test_summarize.py Outdated Show resolved Hide resolved
@Oddant1
Copy link
Member

Oddant1 commented Sep 19, 2023

If only I could order the comments lol. Basically let's make it a Collection[FeatureData[Taxonomy]] not a list that way they can name their entries if they want. Most other comments fall out from that,

@gregcaporaso
Copy link
Member

@Oddant1, @Clockwork-Rat - I have a note on my to-do list to test this out, but it looks like there are some comments still to be addressed. Can you ping me when those are addressed? I'll do my user-level testing/review then.

q2_feature_table/tests/test_summarize.py Outdated Show resolved Hide resolved
q2_feature_table/_summarize/_visualizer.py Outdated Show resolved Hide resolved
@gregcaporaso
Copy link
Member

gregcaporaso commented Sep 22, 2023

@Clockwork-Rat, @Oddant1 - I'm doing some testing of tabulate-seqs and it's working great. I'm experimenting with variations of the following command with the attached files (tab-seqs-tests.zip).

qiime feature-table tabulate-seqs \
 --i-data rep-seqs.qza \
 --i-taxonomy gg:taxonomy.qza \
 --o-visualization rep-seqs.qzv \
 --i-taxonomy something_else:taxonomy.qza \
--m-metadata-file feature-frequency-detail.csv

A few quick requests:

  • add usage example that provides a single taxonomy - let's not name the collection input in this case (e.g., --i-taxonomy taxonomy.qza)
  • add usage example that provides two taxonomies, and illustrate how to give them descriptive names in the example (e.g., --i-taxonomy taxonomy_a:taxonomy-1.qza --i-taxonomy taxonomy_b:taxonomy-2.qza). for the purpose of this example, those two inputs could be silva and greengenes taxonomies, or greengenes 13_8 and greengenes2).
  • validate merge-method value (though that might be a different PR)? - I accidentally ran the following command, and was confused about why I wasn't see the metadata in the output. it's b/c i accidentally provided --p-merge-method instead of --m-metadata-file
# commands succeeds when receiving --p-merge-method but not --m-metadata-file
qiime feature-table tabulate-seqs --i-data rep-seqs.qza --i-taxonomy gg:taxonomy.qza --p-merge-method feature-frequency-detail.csv --o-visualization rep-seqs.qzv --i-taxonomy something_else:taxonomy.qza
Saved Visualization to: rep-seqs.qzv

# Then, I added --m-metadata-file, and it still succeeded - should definitely have thrown an error here:
qiime feature-table tabulate-seqs --i-data rep-seqs.qza --i-taxonomy gg:taxonomy.qza --p-merge-method feature-frequency-detail.csv --o-visualization rep-seqs.qzv --i-taxonomy something_else:taxonomy.qza --m-metadata-file feature-frequency-detail.csv

I'll poke around a bit more and follow up if I notice anything else (just need to run for a meeting). (EDIT: Done - no additional feedback!)

Great work - this is going to be so handy to have!

@gregcaporaso
Copy link
Member

Weirdly, I just noticed the column headers for the Sequence and Sequence length columns are mixed up. Not sure if that was there before or introduced here, but we should fix that in this PR.

Screenshot 2023-09-22 at 11 00 19 AM

@Oddant1
Copy link
Member

Oddant1 commented Sep 22, 2023

@gregcaporaso that should all be doable including the validation on --p-merge-method. I probably should have noticed that. We can just add % Choices

@Oddant1 Oddant1 merged commit daa5357 into qiime2:dev Sep 26, 2023
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

3 participants