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

ENH: Add read length seven-number summary #84

Merged
merged 4 commits into from
Jun 20, 2018

Conversation

jakereps
Copy link
Member

@jakereps jakereps commented Jun 8, 2018

Fixes #71


Wasn't sure where to place the results, so I ended up just putting it on the main overview page.

Previews:

  • Moving Pictures (heavily chopped fastqs)

image

  • Atacama

image


TODO:

  • Add total sample count

image

@jakereps jakereps requested a review from thermokarst June 8, 2018 04:20
@thermokarst thermokarst added this to Backlog (Automated) in 2018.6 via automation Jun 8, 2018
@thermokarst thermokarst moved this from Backlog (Automated) to In Review in 2018.6 Jun 18, 2018
@thermokarst thermokarst self-assigned this Jun 18, 2018
Copy link
Contributor

@thermokarst thermokarst left a comment

Choose a reason for hiding this comment

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

Thanks @jakereps! A few minor requests inline. This looks great, and certainly helps with the 'pre-joined' issue described here: https://docs.qiime2.org/2018.4/tutorials/read-joining/#viewing-a-summary-of-joined-data-with-read-quality

@@ -95,6 +95,17 @@ def _compute_stats_of_df(df):
return df_stats


def _build_seq_len_table(qscores: pd.DataFrame) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make these counts integers?

screen shot 2018-06-19 at 7 05 50 am

Maybe we could also tack on some units, too?

screen shot 2018-06-19 at 7 08 32 am

<div class="row">
<div class="col-lg-12">
<h1>Per-sample sequence counts</h1>
<h4>Total Samples: {{ n_samples }}</h4>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add the total reads (before subsampling)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would this be relocating or in addition to the first table's Total row?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not a smart man. I think I got confused while reviewing - please disregard!

@@ -25,9 +25,28 @@ <h1>Demultiplexed sequence counts summary</h1>
</div>
{% endif %}

<div class="row">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this table get moved into the Quality Plots tab? You can probably get away with just sticking this in as a new row near the bottom of the page (maybe?)

@jakereps
Copy link
Member Author

Thanks, @thermokarst 👍 Just one clarifying question.

@thermokarst
Copy link
Contributor

Thanks @jakereps! This looks great!

Link to q2viewable test viz

@thermokarst thermokarst assigned gregcaporaso and unassigned jakereps Jun 20, 2018
@thermokarst thermokarst moved this from In Development to In Review in 2018.6 Jun 20, 2018
@jakereps
Copy link
Member Author

TIL Service Workers are live on iOS Safari! That link fully worked on mobile. 📱🏆

@thermokarst thermokarst merged commit ac3346b into qiime2:master Jun 20, 2018
2018.6 automation moved this from In Review to Completed Jun 20, 2018
@ebolyen ebolyen removed the request for review from gregcaporaso June 20, 2018 23:30
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
2018.6
  
Completed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants