AlexsLemonade / OpenPBTA-analysis

The analysis repository for the Open Pediatric Brain Tumor Atlas Project
Other
99 stars 66 forks source link

Figure 1: analysis/manuscript workflow overview panel + panel for potential path of analytical PR #1292

Closed jaclyn-taroni closed 2 years ago

jaclyn-taroni commented 2 years ago

Purpose/implementation Section

Addresses https://github.com/AlexsLemonade/OpenPBTA-manuscript/issues/83. Adding the following panels:

Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.

Notes on icon/logo usage

But those guidelines both mention contacting for permission if that’s a route we want to go.

Is the analysis in a mature enough form that the resulting figure(s) and/or table(s) are ready for review?

Yes

⚠️ filing as a draft for now because there’s no docs yet, but I'll still take feedback on the panels themselves.

jharenza commented 2 years ago

Hi @jaclyn-taroni! This looks really nice!

I have a few comments.

For workflow overview:

For analytical PR review:

sjspielman commented 2 years ago

The docker icon being used is a little confusing because it looks so much like a real software of some kind, instead of a "container stub." I wonder if something like a simple "box" would look less that way, but then again anything can be a minimalist icon.. Something like these is what I'm thinking. https://thenounproject.com/icon/box-3820378/ or https://thenounproject.com/icon/box-713425/ (searched "box").

I think it's worthwhile to reach out to docker and CI to see if we can use. We have time before we need the final submission version!

jharenza commented 2 years ago

I think it's worthwhile to reach out to docker and CI to see if we can use.

Agree and also meant to say that!

jaclyn-taroni commented 2 years ago

With https://github.com/AlexsLemonade/OpenPBTA-analysis/pull/1292/commits/18ab6ab854eb46227d976877e9eb6714bead05d0, I've:

It is written linearly, but the process can be very different each time, and I feel like there is sometimes some back and forth, so this might be hard to capture generally. I might have to see some description text to better judge the actual path here. While we want to describe the process, I also think we don't want to make it too complicated, so would you be able to write this out a bit?

Personally, I don't think making it as complicated as it sometimes is in reality is helpful to our readers. So we could add this as is for now, write up some text in the manuscript and revisit.

sjspielman commented 2 years ago

So we could add this as is for now, write up some text in the manuscript and revisit.

I agree with this approach.

jharenza commented 2 years ago

Personally, I don't think making it as complicated as it sometimes is in reality is helpful to our readers. So we could add this as is for now, write up some text in the manuscript and revisit.

Agreed! I meant this seems complicated now, and making it more like reality is going to be even more complicated. Maybe we can simplify this example some more? Although I meant maybe you have a clear description in mind that would make this less complicated to say a clinician reading this.

jharenza commented 2 years ago

Removed the reference to GitHub issues (just "issue" now) so we're at the same level of specificity and don't add more text to the other parts of the panel "Scientific and analytical code review" - to me "analytical code review" means you're reviewing analysis and code that performs the analysis Made the container icon black. If we think the CI and container icons might be placeholders, then this seems fine to me for now!

This looks good!

jaclyn-taroni commented 2 years ago

Only adding docs and figure panels that are not programmatically generated here, so merging without CI!