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

New AMR chapter #95

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

New AMR chapter #95

wants to merge 1 commit into from

Conversation

msberends
Copy link

@msberends msberends commented Feb 1, 2022

This is the start of a new chapter on antimicrobial resistance (AMR) data analysis.

Comments by Neale Batra (by email):

  • The title of the chapter should have one hash (#) and all subheadings below should use two or more (##). This allows the chapter to be properly combined with the other chapters in the Handbook via bookdown.
  • No YAML header needed at all. Just begin with the top-level # title. A common YAML is defined and used for all chapters. Your name will be added to the authors on the front page of the whole handbook.
  • To optimize the look in the ToC, the chapter title will probably need to be shortened to just "Antimicrobial Resistance (AMR)", and it will be placed in the Analysis section of the book.
  • For alignment with the nomenclature of other chapters, can you please organize into these top-level headings (##): Overview, Preparation (with sub-headings "### Load Packages" and "### Import data"), then any other ## headings you want, and finally end with "## Resources". You can explain and load the example data in the "Import data" heading.
  • Can you load the packages with pacman::p_load() instead of library()? You can see the standard paragraph near the top of the other chapters that you can copy, describing why.
  • Taxonomic properties - is there a complete list of these options/functions available in the package? If not too long, can you include the whole list for users of the offline version?
  • The two example datasets aside, there are many instances in this chapter where you create a tibble using code, and then modify the tibble to demonstrate a feature of the AMR package. This differs from how we've approached example datasets in other chapters, where we load example datasets at the top (which are available for download from our repo or R package) and then process and analyze them. While I'm open to some code-created tibbles in the chapter, I'd prefer if we try using a minimal number of datasets introduced at the top. Is this possible?
  • Please try to reserve bolded text for referring to an R package. Use italics instead to emphasize phrases or terms.
  • When loading the two example datasets: I know it's basic, but can you inform the audience in a quick explicit phrase here that they can access these datasets once the AMR package is loaded. Some people may be unfamiliar with accessing datasets stored in packages and wonder what is going on.
  • To display data frames/tibbles for the reader, use DT as below instead of as_tibble() so the display becomes scrollable. You can adjust the pagelength as desired:
    DT::datatable(head(sample_data, 50), rownames = FALSE, options = list(pageLength = 5, scrollX=T), class = 'white-space: nowrap' )
  • Can the data frame that you create that contains antibiotic column names be shown independently (raw) - before you pipe into set_ab_names()?

disks and mics section:

  • Is there any way to have these examples use a data frame loaded at the top of the chapter? Isolated vectors introduce a level of abstraction that we'd like to avoid if possible.
  • In the log2() chunk - can you add brief # comments to the right side of each line (or at least that first line) to explain what !is.na() is doing
  • We should also add a link to the R Basics page section that explains about brackets [] as a subsetting tool
  • the messy_data tibble - again, to avoid scary code let's consider if we can create this data frame in advance and store it for download?
  • Selecting and filtering with selectors - super cool feature! We can link to the section that introduces tidyverse selectors. I think this first appears in the Cleaning page.
  • Can you think about whether for some chunks the messages=FALSE argument would be prudent? Perhaps there are some messages that appear that should be shown, but others that are unnecessary.
    • For example: ## i Using column 'mo' as input for 'col_mo'.
  • We can make the plots more visible with chunk parameters like:
    • out.width=c('50%'), and
    • fig.show='hold', if you want two align a couple plots side-by-side to conserve vertical space

@netlify
Copy link

netlify bot commented Feb 1, 2022

✔️ Deploy Preview for epirhandbook ready!

🔨 Explore the source changes: 60e56e5

🔍 Inspect the deploy log: https://app.netlify.com/sites/epirhandbook/deploys/61f8eb88a046830007a8d782

😎 Browse the preview: https://deploy-preview-95--epirhandbook.netlify.app

@msberends
Copy link
Author

I'll fix your comments first @nsbatra, then we can discuss here about how to proceed if you're okay with that 🙂

Also easier to comment per line of code of course now that there's a separate branch.

@@ -68,6 +68,7 @@ rmd_files: [
"new_pages/combination_analysis.Rmd",
"new_pages/transmission_chains.Rmd",
"new_pages/phylogenetic_trees.Rmd",
"new_pages/AMR.Rmd",
Copy link
Author

@msberends msberends Feb 1, 2022

Choose a reason for hiding this comment

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

In the mail you advised to put it below Phylogenetic Trees in the Analysis section. But Phylogenetic Trees is not in the Analysis section, it's in the Data Visualization section. So shall we put AMR in the Analysis section, or do you want to keep it here? Both fine to me, although I consider this chapter to be more about analysis than about dataviz.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah you are right! I misread. Agree analysis section is best. For now let's just put it at the end of the Analysis section.
Thanks for setting up this PR!

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