Skip to content

32 update pipeline metro map on the readme page#36

Merged
heylf merged 9 commits into
masterfrom
32-update-pipeline-metro-map-on-the-readme-page
Dec 3, 2024
Merged

32 update pipeline metro map on the readme page#36
heylf merged 9 commits into
masterfrom
32-update-pipeline-metro-map-on-the-readme-page

Conversation

@khersameesh24
Copy link
Copy Markdown
Collaborator

PR checklist

  • 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/spatialxe 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).

@khersameesh24 khersameesh24 linked an issue Nov 25, 2024 that may be closed by this pull request
@khersameesh24 khersameesh24 requested a review from heylf November 25, 2024 12:27
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please also provide an svg version. makes it easier to edit it in the future.

Looks very nice, some small nit-picking:

  • The orange and the green are the same kind of brown for red-green-sight-deficiant people.
  • try to center seggar and resegment nodes inbetwen the curves (also resegment could need some padding to the left)
  • you have enough space for cellpase and proseq to be actually next to their node.
  • try to make the gray boxes have the same distance to the green line after baysor
  • I would try to make the relabel process in the same hight as 5the coordinates/mask one, makes it easier to see that it is actually another input path.
  • speaking of input: their symbol is not expalined in the legend
  • I don't know if the arrows are actually needed. try if everything is still readable without them.
  • the folder for xenium bundle is not centered, smae with the ro-crate symbol to the right and the spatialdata icon to the left of coordinate mask. similarly, the gen epanel json icon and the html qc reports are not centeredd horizontally. and the first coorcinates/mask is neither.
  • thinking about the color usage. what if instead of approach the encode the original data. so you could use blue for transcripts and orange for morphology and drop green and just combine the two after the baysor step and thereafter

but again this are just small things the whole thing looks very clear, congrats!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hi @mashehu, I have made the suggested changes and updated the metromap. Also, added the svg for the metromap.

@heylf heylf mentioned this pull request Nov 27, 2024
9 tasks
@heylf
Copy link
Copy Markdown
Collaborator

heylf commented Nov 29, 2024

@khersameesh24 one tiny remark that I think should be changed is the input of the relabeling what mashehu already mentioned. It is not clear that the json file for the relabeling is an input. Can you just add another arrow there please?

Or for the version without arrow, put the json file above and not on the same height at the output files.

@khersameesh24
Copy link
Copy Markdown
Collaborator Author

For the gene panel json file and all other inputs the color coding is gray but for the output files its white. Also, the legend has an input and output key. I tried getting it on the other side to fix the height, but it does not look very good.

@mashehu mashehu requested a review from jfy133 November 29, 2024 10:29
@heylf
Copy link
Copy Markdown
Collaborator

heylf commented Dec 2, 2024

Then I would really say to just add another arrow. Sombeody can miss the grey quite easily.

…' into 32-update-pipeline-metro-map-on-the-readme-page
…p-on-the-readme-page

32 update pipeline metro map on the readme page
Copy link
Copy Markdown
Collaborator

@heylf heylf left a comment

Choose a reason for hiding this comment

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

LGTM

@heylf heylf merged commit e89c1d9 into master Dec 3, 2024
@khersameesh24 khersameesh24 deleted the 32-update-pipeline-metro-map-on-the-readme-page branch May 5, 2025 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update pipeline metro map on the README page

3 participants