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

Add main figure 3 #42

Merged
merged 5 commits into from
Jun 3, 2024

Conversation

jenna-tomkinson
Copy link
Member

Add main figure 3

In this PR, the model data was updated that will be used for model evaluation and generated main figure 3 for the manuscript/presentation.

@jenna-tomkinson jenna-tomkinson requested a review from gwaybio May 30, 2024 19:36
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@gwaybio
Copy link
Member

gwaybio commented May 31, 2024

Figure nitpicks incoming!

Panel A

  • There's no need to specify "all_plates" in the facet (no need to facet at all)
  • The font size is a bit small everywhere, particularly the labels above each bar
  • Please add context to the grouped bar labels. "Final" doesn't mean much to someone who is unfamiliar with the work. Can you think of a different word to describe this? Similarly, "Shuffled" doesn't mean much either. What is shuffled? Perhaps labels "NF1 genotype\npredictions" and "Negative control" 🤷 ?
  • The x axis label is "Data Type", but this isn't specific enough either - maybe no label is the way to go?
  • The legend title is "Data Splits", but this is also a bit vague. Perhaps "Machine learning\nmodel data\nsplits"?
  • While "rest" and "test" make sense for us internally, it is ok to change for the public-facing visualization. I would go back to "Train" and "Test".
  • Note the consistent capitalization throughout my suggestions above :)

Panel B

  • Make sure to transfer consistent terminology in Panel B when you update Panel A
  • Same comment about the facet too

Panel C

  • Make sure to transfer consistent terminology in Panel C when you update Panels A/B (e.g., test/rest)
  • Whitespace management probably can be improved. Is there another useful panel you can make for D? Maybe example image? Or I might recommend making C on the same row (not two rows)
  • The font size and boxes can prob be bigger (especially font size within boxes)
  • Camel case "Predicted Genotype" should be title case ("Predicted genotype")

@jenna-tomkinson
Copy link
Member Author

@gwaybio I have addressed the first round of nit-picks! 😄 Feel free to re-review when possible!

Copy link
Member

@gwaybio gwaybio left a comment

Choose a reason for hiding this comment

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

LGTM! Here are some final minor nitpicks

  • The precision recall curve is not coord_fixed() (or close to having the same x and y axis distance - it should be close to square). You can shrink the height of the top row in the patchwork to make it more square, or, you can swap panels B and C
  • The whitespace in the confusion matrix is still a bit off between squares. Specifically, the horizontal distance is about 2x greater than the vertical distance (e.g., in top left, the distance between 885 and 227 is ~2x greater than 885 and 214.)

This plot looks great otherwise, feel free to merge at will!

@jenna-tomkinson
Copy link
Member Author

@gwaybio I have finished the second round of revisions and I will merge now! Thank you for the reviews!

@jenna-tomkinson jenna-tomkinson merged commit c5be97d into WayScience:main Jun 3, 2024
@jenna-tomkinson jenna-tomkinson deleted the add_main_figure_3 branch June 3, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants