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

merge fastq_stats_* visualizers into a single action #70

Closed
nbokulich opened this issue May 29, 2020 · 1 comment · Fixed by #71
Closed

merge fastq_stats_* visualizers into a single action #70

nbokulich opened this issue May 29, 2020 · 1 comment · Fixed by #71
Projects

Comments

@nbokulich
Copy link
Member

Improvement Description
Now that we have a brand-new unified view type for SE and PE fastq formats, fastq_stats_single and fastq_stats_paired could be merged into a single action. See:
qiime2/q2-types#245
and here for an open PR where this unified view type is utilized:
qiime2/q2-quality-control#52

Current Behavior
fastq_stats_single and fastq_stats_paired exist as separate visualizers. I am excited for these new visualizers! We have an opportunity to merge these into one before they area released to make use more straightforward.

Proposed Behavior
Merge these visualizers into one fastq_stats visualizer. This can probably be done by:

  1. changing the view type here to CasavaOneEightSingleLanePerSampleDirFmt (that format has a new manifest property so afaik this line should still work but needs testing)
  2. instead of this if statement you could then just check if the manifest has a reverse column.
  3. in plugin_setup.py you would register a union type as the input type. E.g., instead of specifying SampleData[PairedEndSequencesWithQuality] as seen here you would do SampleData[SequencesWithQuality | PairedEndSequencesWithQuality]

Then just cleaning up the docs, tests etc to use the unified action. (I may be missing a few other steps, but the above should be the gist of the main changes I think)

@nbokulich nbokulich added this to Backlog in 2020.6 via automation May 29, 2020
@angrybee angrybee self-assigned this Jun 1, 2020
@angrybee
Copy link
Member

angrybee commented Jun 1, 2020

I took a look, it works nicely but the unittests have to be rewritten. I will open a PR this week.

2020.6 automation moved this from Backlog to Changelog Needed Jun 9, 2020
@angrybee angrybee removed their assignment Jun 10, 2020
@ChrisKeefe ChrisKeefe moved this from Changelog Needed to Completed in 2020.6 Jun 17, 2020
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
2020.6
  
Completed
Development

Successfully merging a pull request may close this issue.

2 participants