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

Bioconductor/sweave2rmd: Conversion of howtogenefilter.Rnw to Rmd #11

Merged
merged 31 commits into from
Mar 28, 2023

Conversation

Khadeeejah
Copy link
Contributor

@Bioconductor/sweave2rmd
My name is Khadijah an Outreachy applicant. I was assigned this issue on the project board,

@jwokaty Kindly review my PR.

HTML
PDF

@Khadeeejah
Copy link
Contributor Author

@Bioconductor/sweave2rmd please review my contribution for Outreachy.
@jwokaty

@jwokaty
Copy link
Contributor

jwokaty commented Mar 13, 2023

Hi @Khadeeejah, Thanks for your pull request. We're going to use the following checklist for the review, doing a little bit at a time:

  • the .Rmd file knits to HTML
  • R CMD build runs without errors or timeouts
  • the tarball from R CMD build contains the HTML (check with the
    following by substituting the package name and vignette name tar ztf package_name.tar.gz | grep 'doc/vignette_name')
  • the .Rnw file has been removed
  • in the DESCRIPTION, BiocStyle and knitr are listed in Suggests
  • in the DESCRIPITON, the line VignetteBuilder: knitr exists
  • any added lines use the same spacing and indents as the existing
    document
  • if agreed with the Maintainer, the contributor is in the author list
    in the DESCRIPTION file; refer to the contribution guide for acceptable
    formats
  • if agreed with the Maintainer, the contributor is in the author list
    in the vignette's YAML; refer to the contribution guide for acceptable formats
  • if this pull request includes a conversion from using the separate Author
    and Maintainer lines into the Authors@R vector, check that the
    Authors@R vector includes the maintainer as specified with role='cre'.
  • if this pull request includes a conversion from using the separate Author
    and Maintainer lines into the Authors@R vector, the Maintainer line
    is completely removed.
  • HTML document is representative of the PDF in content and
    in general the presentation
  • Where the contributor was not able to preserve the content and presentation
    of the PDF is noted as a comment in the pull request
  • the R Markdown file is representative of the Sweave document and follows
    best practices, such as replacing links to Bioconductor packages with calls
    to BiocStyle's Biocpkg()

DESCRIPTION Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
@jwokaty
Copy link
Contributor

jwokaty commented Mar 13, 2023

@Khadeeejah Please address these changes. I believe some of them, such as the inline code or blank spaces may appear again later in the document. Please also address those. Push your changes to this PR and let me know we're ready to continue the review.

@Khadeeejah
Copy link
Contributor Author

@jwokaty thank you
I'll do everything right away ☺️

@Khadeeejah
Copy link
Contributor Author

@jwokaty kindly review i have fixed all

DESCRIPTION Outdated Show resolved Hide resolved
@Khadeeejah
Copy link
Contributor Author

@jwokaty i have updated my pull request
kindly check out my HTML FILE

@Khadeeejah
Copy link
Contributor Author

@jwokaty

@jwokaty
Copy link
Contributor

jwokaty commented Mar 23, 2023

@Khadeeejah Can you please wrap long lines and check the correct syntax for code block names in the R Markdown cheatsheet?

DESCRIPTION Outdated Show resolved Hide resolved
Copy link
Contributor

@jwokaty jwokaty left a comment

Choose a reason for hiding this comment

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

@Khadeeejah Hi, I have marked additional issues, which are mostly minor, but these should be the final issues addressed before we can merge. Although I have mentioned it in another comment, please wrap all long lines and correct the naming of code blocks (there should be no comma between the r and the name of the code block.

@jwokaty
Copy link
Contributor

jwokaty commented Mar 24, 2023

@Khadeeejah Can I ask that you leave these unresolved? I will use them to recheck your code when you have pushed all your changes?

@Khadeeejah
Copy link
Contributor Author

Good morning ma @jwokaty i have updated my PR.

Comment on lines 64 to 70
Here `f1` is a function that implies our "expression measure above 200 in at
least 5 samples" criterion, the function `ffun` is the filtering function (which
in this case consists of only one criterion), and we apply it using
`r Biocpkg("genefilter")`. There were `r sum(wh1)` genes that satisfied the
criterion and passed the filter.
As an example for a specific filter, let us select genes that are differentially
expressed in the groups defined by `type`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you reformat lines 64-70 and 78-84?

@jwokaty
Copy link
Contributor

jwokaty commented Mar 27, 2023

Hi @Khadeeejah, I have 2 last changes to improve the formatting of 2 paragraphs. After you push the changes, provided that there are no new issues, I'll merge your PR.

@Khadeeejah
Copy link
Contributor Author

@jwokaty i have formatted the two paragraphs, thank you

@jwokaty jwokaty merged commit 0970197 into Bioconductor:devel Mar 28, 2023
@jwokaty
Copy link
Contributor

jwokaty commented Mar 28, 2023

Thanks for your contribution! I'm merging your PR.

@Khadeeejah
Copy link
Contributor Author

Thanks for your contribution! I'm merging your PR.

Thank you so much please can you assign the last vignette on the board to me or any other

I would love to contribute more

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.

None yet

3 participants