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.

Sign up for GitHub

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

Jump to bottom

Add Pipeline diagram #812

Merged
merged 22 commits into from
Apr 29, 2022
Merged

Add Pipeline diagram #812

merged 22 commits into from
Apr 29, 2022

Conversation

jfy133
Copy link
Member

@jfy133 jfy133 commented Apr 26, 2022

PR checklist

Originally drawn by @G-Sarah

As requested by @drpatelh

Closes #809

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
    • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
    • If necessary, also make a PR on the nf-core/rnaseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@jfy133 jfy133 changed the base branch from master to dev April 26, 2022 19:45
@github-actions
Copy link

This PR is against the master branch ❌

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @jfy133,

It looks like this pull-request is has been made against the jfy133/nf-core-rnaseq master branch.
The master branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to master are only allowed if they come from the jfy133/nf-core-rnaseq dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

@jfy133
Copy link
Member Author

jfy133 commented Apr 26, 2022

@G-Sarah please could you update the CHANGELOG on my fork so you make sure you're added to the repo as a contributor

Also I would need a description for the various line colours, so I can make a legend :)

@github-actions
Copy link

github-actions bot commented Apr 26, 2022 •
edited
Loading

nf-core lint overall result: Passed ✅

Posted for pipeline commit 690c38d

+| ✅ 144 tests passed       |+
#| ❔   3 tests were ignored |#

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: assets/email_template.html
  • files_unchanged - File ignored due to lint config: assets/email_template.txt
  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy

✅ Tests passed:

Run details

  • nf-core/tools version 2.3.2
  • Run at 2022-04-29 08:45:46

@G-Sarah
Copy link
Contributor

G-Sarah commented Apr 27, 2022

Done, thank you! Is that what you had in mind?
Also, for the legend I'd put something like :

  • In fushia, route for pseudomapping with Salmon
  • In orange, route for mapping with HiSAT2 (no quantification)
  • In green, route for mapping with STAR and quantification with Salmon (default)
  • In blue, route for mapping with STAR and quantification with RSEM

@jfy133
Copy link
Member Author

jfy133 commented Apr 27, 2022

Yes, perfect!

Awesome - I will try and add that tonight :)

@drpatelh
Copy link
Member

drpatelh commented Apr 27, 2022 •
edited by jfy133
Loading

This is awesome! I am going to print this out and maybe draw comments on it. But for now:

Colours

Would be awesome to change the colours to match the real-tube map as linked to here. Would need to see how it looks but maybe something like this?

  • Pseudo-mapping with Salmon: Victoria line #0098D4 / Picadilly line #003688
  • HiSAT2 (no quantification): Metropolitan line #9B0056
  • STAR and quantification with Salmon (default): Central line #E32017
  • STAR and quantification with RSEM: District line #00782A

Grey boxes

  • Instead of putting text like Pre-processing into the boxes can we put numbers like in the tube map and maybe have a key for this too?
  • Instead of straight edges for the grey boxes can we have edges like below?
  • Change the colour code for the grey box to match the original tube map #E7E7E7

image

Other

  • The stop for HISAT2 is below the lines.

@G-Sarah
Copy link
Contributor

G-Sarah commented Apr 27, 2022

Following up on the @mashehu 's comment on Slack, I played around with the colors @drpatelh requested to make them more colorblind-friendly:

  • #002B6B Pseudo-mapping with Salmon
  • #E40481 HiSAT2 (no quantification)
  • #FFA140 STAR and quantification with Salmon (default)
  • #01862E STAR and quantification with RSEM
    I think they look pretty distinct while retaining as much of the spirit of the requested colors as possible.

@jfy133 jfy133 added the WIP Work in progress label Apr 27, 2022
@jfy133
Copy link
Member Author

jfy133 commented Apr 27, 2022 •
edited
Loading

pipeline_diagram_grey

Updated colours and adopted the zone numbering system.

@drpatelh the zone border curves are tricky to get right, and you don't really have in your map sufficient 'shapes' to make it worth it (IMO). As a compromise I've reintroduced the curves that @G-Sarah had originaly but made them smaller so they look a tad more 'neater' and consistent

mashehu
mashehu reviewed Apr 27, 2022
View reviewed changes
Copy link
Contributor

@mashehu mashehu left a comment

Choose a reason for hiding this comment

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

I would either remove the gray background or make it span the whole graph, without gaps. It will look otherwise look of on certain background, for example in dark mode. Similarly, I would use the nf-core/rna-seq logo without a white background so it fits better into the whole picture.

@jfy133
Copy link
Member Author

jfy133 commented Apr 27, 2022 •
edited
Loading

I would either remove the gray background or make it span the whole graph, without gaps. It will look otherwise look of on certain background, for example in dark mode. Similarly, I would use the nf-core/rna-seq logo without a white background so it fits better into the whole picture.

Damn you were too fast ;)

I'd rather just embed a white background. If we want to go full darkmode support we have to play with the colours even more

@mashehu
Copy link
Contributor

mashehu commented Apr 27, 2022

oh, sorry didnt' see the number in the gray areas in the transparent preview. 🙃
You have enough space to write the names of the areas actually, would be easier to read and orientate oneself.

@jfy133
Copy link
Member Author

jfy133 commented Apr 27, 2022 •
edited
Loading

oh, sorry didnt' see the number in the gray areas in the transparent preview. upside_down_face You have enough space to write the names of the areas actually, would be easier to read and orientate oneself.

Agreed, but @drpatelh requested the numbers (I think he's been travelling on the underground too long so maybe wants it to look really similar to the real thing ;) ), lets see what he says

@mashehu
Copy link
Contributor

mashehu commented Apr 27, 2022

oh, sorry didnt' see the number in the gray areas in the transparent preview. upside_down_face You have enough space to write the names of the areas actually, would be easier to read and orientate oneself.

Agreed, but @drpatelh requested the numbers (I think he's been travelling on the underground too long so maybe wants it to look really similar to the real thing ;) ), lets see what he says

as somebody who doesn't have to check zones on maps, I would have not made the connection... but maybe switch the numbers 2 and 3 so they follow left-to-right and top-to-bottom reading direction.

@mashehu
Copy link
Contributor

mashehu commented Apr 27, 2022

you could enhance the readability of the numbers by adding more padding around them, so there is a bigger gray gap around them

@jfy133
Copy link
Member Author

jfy133 commented Apr 27, 2022

Updated in the original screenshot!

@G-Sarah
Copy link
Contributor

G-Sarah commented Apr 27, 2022 •
edited
Loading

I also think that the names directly written in the cells were easier to read, though it's true that it looks less like the londonian tube.
Careful, the HiSAT and STAR + Salmon routes are swapped in the legend! I really like the look of it though!

PS: That's my fault I didn't put the right order in my last comment 😅

@jfy133
Copy link
Member Author

jfy133 commented Apr 27, 2022

I also think that the names directly written in the cells were easier to read, though it's true that it looks less like the londonian tube. Careful, the HiSAT and STAR + Salmon routes are swapped in the legend! I really like the look of it though!

PS: That's my fault I didn't put the right order in my last comment sweat_smile

Fixed :)

@drpatelh
Copy link
Member

drpatelh commented Apr 27, 2022 •
edited
Loading

AWEEESSOMMMEE!!!

Ok. Not sure which of these can and can't be addressed but Round 2 of comments.

General

  • Add even amount of whitespace around the entire image
  • Can we make the logo bigger and push the legends towards the bottom. May need to find a home for the license bit.
  • Swap BBSplit with SortMeRNA
  • Swap DESeq2 (PCA only) with dupRadar
  • SAMTools -> SAMtools
  • BedgraphtoBigWig -> bedGraphToBigWig (can we squeeze this on one line?)
  • Change the GFF input icon to GTF because that is the standard even though the pipeline can accept GFF too.
  • Change all file icons to use uppercase:
    • fastq to FASTQ
    • fasta to FASTA
    • bigWig to BIGWIG

METHOD legend

  • It should be STAR and quantification with Salmon (default)
  • Move STAR and quantification with Salmon (default) to the top because it is the default route
  • HiSAT2 (no quantification) -> HiSAT2 with no quantification
  • Pseudo-mapping and not Pseudo-Mapping

STAGE legend

  • Genome alignment -> Genome alignment and quantification
  • Pseudo-alignment -> Pseudo-alignment and quantification

Going to look at the inputs/outputs of the pipeline to make sure we are capturing everyting. Thanks guys!

@jfy133
Copy link
Member Author

jfy133 commented Apr 28, 2022

pipeline_diagram_grey

OK'd version from @drpatelh

@G-Sarah please update/let me know if you want a different credit attribution!

@G-Sarah
Copy link
Contributor

G-Sarah commented Apr 28, 2022

@G-Sarah please update/let me know if you want a different credit attribution!
No that's perfect thank you! Great job on the diagram, it really went a long way and looks incredible! ✨

@drpatelh
Copy link
Member

Some weirdness in the rendering @jfy133 that wasn't there for previous versions?

https://github.com/jfy133/nf-core-rnaseq/tree/pipeline-diagram#pipeline-summary

@jfy133
Copy link
Member Author

jfy133 commented Apr 28, 2022

@jfy133
Copy link
Member Author

jfy133 commented Apr 28, 2022

Or you did... wtf...

@drpatelh
Copy link
Member

@jfy133 anything we can do about the end circle being too small here?

image

@jfy133
Copy link
Member Author

jfy133 commented Apr 28, 2022

Feed me cookies!

I'm AFK for kindergarten run now unfortunately. Maybe @G-Sarah can update if available?

@drpatelh
Copy link
Member

drpatelh commented Apr 28, 2022 •
edited
Loading

Feed me cookies!

In the post!

@G-Sarah
Copy link
Contributor

G-Sarah commented Apr 28, 2022

I added a black line to kind of extend the circle, what do you think?
pipeline_diagram_grey

@mashehu
Copy link
Contributor

mashehu commented Apr 28, 2022

I would be less strict with the circle size and just make it four lines tall until the first fork.

@jfy133
Copy link
Member Author

jfy133 commented Apr 28, 2022

You'll need to also make bigger the input fastp circle too for consistency

@G-Sarah
Copy link
Contributor

G-Sarah commented Apr 28, 2022

image
Something like this for stage 1?

@drpatelh
Copy link
Member

drpatelh commented Apr 28, 2022 •
edited
Loading

Spacing is still off in a few places. The file icons don't look evenly spaced relative to the lines / circles either? May need to do a final run-through for the station names to make sure they are good too.

image

@jfy133
Copy link
Member Author

jfy133 commented Apr 29, 2022

  1. I've reduced the size of the output bars to be proportional to the tracks
  2. I've right/left aligned MultiQC (x2) with the bars (can't center them because they collide with the icons.
  3. All the file icons are centered to the station circle/tracks,they look slightly off because more whitspace on the top of the icon
  4. Please send me a list of corrections of the station names, if any

pipeline_diagram_grey

@G-Sarah
Copy link
Contributor

G-Sarah commented Apr 29, 2022

There was a conflict due to an addition to the dev branch changelog, resolved it by keeping both the diagram line and the new line in dev

@drpatelh drpatelh mentioned this pull request Apr 29, 2022
Closed
@drpatelh
Copy link
Member

Thanks again guys! This looks amazing!!

drpatelh
drpatelh approved these changes Apr 29, 2022
View reviewed changes
Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

🤩

@drpatelh drpatelh merged commit 13503cc into nf-core:dev Apr 29, 2022
@jfy133
Copy link
Member Author

jfy133 commented Apr 29, 2022

Good job team 💪

And I still have never actually run the pipeline nor understand what it really does 😂

@G-Sarah
Copy link
Contributor

G-Sarah commented Apr 29, 2022

Yay, great job everyone! \o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mashehu mashehu mashehu left review comments

@drpatelh drpatelh drpatelh approved these changes

Assignees
No one assigned
Labels
WIP Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add metro map for pipeline
4 participants
@jfy133 @G-Sarah @drpatelh @mashehu