Convert byChroms.Rnw to byChroms.Rmd #2

Rohit-Satyam · 2023-03-15T17:49:02Z

Asking for review from @Bioconductor/sweave2rmd @jwokaty. Below attached is the HTML output of the vignette.
byChroms.zip

jwokaty · 2023-03-15T18:39:33Z

vignettes/byChroms.Rmd

BerylKanali · 2023-03-24T20:44:58Z

BerylKanali

Hi @Rohit-Satyam
I made some few additional comments, take a look at them and make the changes.

vignettes/byChroms.Rmd

jwokaty · 2023-03-29T21:59:16Z

@BerylKanali @Rohit-Satyam Have all changes been made and it is ready for me?

BerylKanali · 2023-03-30T12:46:33Z

@jwokaty
Not yet. I had asked @Rohit-Satyam to make some minor changes then it will be good to go.

Removed spaces.

BerylKanali

Hi @Rohit-Satyam
Kindly go through your work one more time to ensure there are no extra spaces. After that let me know.

vignettes/byChroms.Rmd

BerylKanali · 2023-03-31T17:42:50Z

vignettes/byChroms.Rmd

+    BiocStyle::pdf_document: default
+
+---
+


line 27 as well should be removed

Rohit-Satyam · 2023-04-01T14:10:24Z

Should I just use this PR link in to register contribution in Outrechy?

BerylKanali · 2023-04-01T18:06:08Z

Should I just use this PR link in to register contribution in Outrechy?

Yes

mcarlsn · 2023-04-13T17:13:08Z

Asking for review from @Bioconductor/sweave2rmd @jwokaty. Below attached is the HTML output of the vignette. byChroms.zip

@Rohit-Satyam Thanks so much for your work on this conversion. We're in the process of organizing the Sweave2Rmd project board post-Outreachy contribution period ending and I wanted to follow-up with you on this vignette. Are you interested in completing the conversion? If not, no worries - we can pass it along to someone else - but I wanted to check with you first. Let us know!

Rohit-Satyam · 2023-04-27T16:37:22Z

Hi @mcarlsn. I am sorry I am replying this two weeks later and this is because for some reason I am not getting github notifications on my gmails. I will resolve this issue. Besides, yes I would like to complete this package vignette conversion if it has not been assigned to someone else yet. Again apologies.

Edit: I saw your emails were in my spam folder. I sincerely apologize for the inconvenience. I thought since there was no notifications that this pull request might have been successfully merged.

Rohit-Satyam · 2023-04-27T17:44:22Z

I have also added visualize.Rmd as requested here. Kindly review @mcarlsn

jwokaty

Thanks for this PR. We generally keep conversions separate (1 PR to 1 conversion) and in this case, we already have a good PR for visualize.Rmd; however, it is being held because it has an old plot that requires fixing before we can merge it. Unfortunately, I think it is best to remove the visualize files here.

In addition to the items, I've marked, there are two additional issues.

The first is that the plot at the bottom of the vignette has changed over time if you compare it with the vignette in Bioc 2.0--and this is not anything I would expect you to catch; I only guessed it might have a problem because the other vignette has a problem. I made a suggestion below that might get us closer, but I think I might have to ask for help to update it to get the X, M, and Y parts unless you can figure it.

The second problem is that running R CMD build doesn't generate the html in the doc path, which is how the build system will generate it. After removing visualize, it might be easier to troubleshoot the issue.

jwokaty · 2023-05-06T03:27:59Z

vignettes/byChroms.Rmd

@@ -0,0 +1,90 @@
+---
+title: "geneplotter:Graphics related functions for Bioconductor"


Change the title to How to assemble a chromLocation object.

jwokaty · 2023-05-06T03:29:31Z

vignettes/byChroms.Rmd

+package: geneplotter
+
+vignette: >
+ %VignetteIndexEntry{geneplotter:Graphics related functions for Bioconductor}


Generally, the VignetteIndexEntry is the same as the title. Please change to How to assemble a chromLocation object.

vignettes/byChroms.Rmd

jwokaty · 2023-05-06T03:45:47Z

vignettes/byChroms.Rmd

+And finally we can test it by calling `cPlot`.
+```{r cPlot, fig=TRUE}


Add a blank line between 86 and 87.

vignettes/byChroms.Rmd

jwokaty · 2023-05-06T04:52:33Z

vignettes/byChroms.Rmd

+And finally we can test it by calling `cPlot`.
+```{r cPlot, fig=TRUE}
+  library(geneplotter)
+  cPlot(newChrClass)


This plot is old and it's no longer correct if we look at the version from Bioconductor 2.0: https://bioconductor.org/packages/2.0/bioc/vignettes/geneplotter/inst/doc/byChroms.pdf. We can kind of get closer to it by doing cPlot(newChrClass, as.character(1:22)) but we also need X, M, Y, so we might have to ask for additional help to fix this plot.

Hi Pardon my ignorance. I thought it was due to update in related packages and changes in the plot was expected. But I can offer additional help as well. The code can be modified as follows to generate the exact plot.

library(geneplotter) ## Reorder Chromosomes newChrClass@chromLocs <- newChrClass@chromLocs[order(as.numeric(names(newChrClass@chromLocs)))] newChrClass@chromInfo <- newChrClass@chromInfo[order(as.numeric(names(newChrClass@chromInfo)))] cPlot(newChrClass,useChroms = as.character(c(names(splits),"X","Y","M")))

Thanks for fixing the plot.

jwokaty · 2023-05-06T05:37:45Z

vignettes/byChroms.Rmd

+ %VignetteDepends{annotate, hu6800.db}
+ %VignetteKeywords{chromosomes}
+ %VignettePackage{geneplotter}
+ %VignetteAuthor{Robert Gentleman}


Lines 13-19 should have 2 spaces. Currently there is only 1.

Done. But I don't understand why introducing all these spaces are so important when the Vignette is knitting just fine?

I tried to respond to a similar comment on slack, but I'll respond here. The reason why we're converting vignettes to .Rmd is that .Rmd files are easier to maintain, but just because something is in .Rmd format doesn't mean it's automatically readable so the requests to correct spacing, wrapping long lines, etc., are style guidelines that we've been picking up along the way as we refine the sweave2rmd process. Some of these suggestions come from core members, such as reformatting long lines. Because we also work with maintainers on their packages and their standards can be quite different, possibly stricter, our PRs need to be in the best shape possible.

This style guide really needs to be added to the documentation. I also hope that in the future, we'll have a package do all or most of this work for us.

vignettes/byChroms.Rmd

Removal of this vignette requested in [PR](Bioconductor#2)

As per [PR](Bioconductor#2)

Rohit-Satyam · 2023-05-06T18:51:35Z

The improved vignette is attached below.
byChroms.zip

jwokaty

Can you add visualize.Rnw back in?

I still cannot find the html in the doc path after building the vignette, which is how it will be generated by the build system. I am not sure where the problem is.

When I run BiocCheck::BiocCheck on the package, I get the following:

* Checking parsed R code in R directory, examples, vignettes...
    Found @ in vignettes/byChroms.Rmd
    * NOTE: Use accessors; don't access S4 class slots via '@' in
      examples/vignettes.

Can you address these issues?

jwokaty · 2023-05-14T05:38:45Z

vignettes/byChroms.Rmd

+  %VignetteIndexEntry{How to Assemble a chromLocation Object}
+  %VignetteEngine{knitr::rmarkdown}
+  %VignetteEncoding{UTF-8}  
+  %VignetteDepends{annotate, hu6800.db}
+  %VignetteKeywords{chromosomes}
+  %VignettePackage{geneplotter}
+  %VignetteAuthor{Robert Gentleman}


These are missing the \; for example

%\VignetteIndexEntry{How to Assemble a chromLocation Object}

This is why we're not getting an error but the html isn't generated in the doc path of the tarball. Could I ask you to fix this here and in the other vignettes? Both GeneMeta and ShortRead have the same problem.

I have made amendments in all the vignettes PR I have opened so far. The PRs are

Convert Overview.Rnw to Overview.Rmd ShortRead#14

Convert summarizeOverlaps.Rnw to summarizeOverlaps.Rmd GenomicAlignments#28

Convert GeneMeta.Rnw to GeneMeta.Rmd GeneMeta#2

Convert byChroms.Rnw to byChroms.Rmd #2

ggbio rmd lawremi/ggbio#168

Enrichmentbrowser rmd lgeistlinger/EnrichmentBrowser#44

Thank you. I've been checking these to see if you updated any. I will look at this PR first. Enrichmentbrowser needs to be reviewed by the maintainer who will merge the PR. (Maybe he needs a gentle reminder?)

Added the missing `\` as suggested by reviewer in [issue](Bioconductor#2 (comment))

Added the visualize.Rnw as per request by reviewer in [issue](Bioconductor#2 (review))

Added space before and after the code block as suggested in Bioconductor/geneplotter#2 (comment)

jwokaty · 2023-06-06T21:10:59Z

@Rohit-Satyam Thanks for your contribution! @BerylKanali Thanks for reviewing!

Rohit-Satyam added 2 commits March 15, 2023 23:03

Converted byChroms.Rnw file to Rmd

536b77a

Update DESCRIPTION with BiocStyle, knitr, VignetteBuilder

e0ad6b2

Rohit-Satyam mentioned this pull request Mar 15, 2023

Replace a Sweave Vignette With an R Markdown Vignette Bioconductor/sweave2rmd#63

Closed

BerylKanali reviewed Mar 24, 2023

View reviewed changes

vignettes/byChroms.Rmd Show resolved Hide resolved

vignettes/byChroms.Rmd Outdated Show resolved Hide resolved

vignettes/byChroms.Rmd Outdated Show resolved Hide resolved

Make changes in .Rmd file

bc4c6c4

BerylKanali suggested changes Mar 27, 2023

View reviewed changes

vignettes/byChroms.Rmd Show resolved Hide resolved

vignettes/byChroms.Rmd Show resolved Hide resolved

vignettes/byChroms.Rmd Show resolved Hide resolved

jwokaty changed the title ~~Geneplotter rmd~~ Convert byChroms.Rnw to byChroms.Rmd Mar 30, 2023

Update byChroms.Rmd

6716aa8

Removed spaces.

BerylKanali suggested changes Mar 31, 2023

View reviewed changes

Rohit-Satyam added 2 commits April 27, 2023 22:20

Removed extra spaces

2e8df00

Adding visualize.Rmd

8800c4c

Edited Authors contribution section

5de89c1

jwokaty requested changes May 6, 2023

View reviewed changes

Rohit-Satyam added 2 commits May 7, 2023 00:18

Removal Requested

4a6d651

Removal of this vignette requested in [PR](Bioconductor#2)

Minor revision in Vignette Rmd

6aa4bbc

As per [PR](Bioconductor#2)

jwokaty requested changes May 13, 2023

View reviewed changes

jwokaty reviewed May 14, 2023

View reviewed changes

Rohit-Satyam added 2 commits June 2, 2023 22:33

Update byChroms.Rmd

7082cef

Added the missing `\` as suggested by reviewer in [issue](Bioconductor#2 (comment))

Adding back visualize.Rnw

46bb3b4

Added the visualize.Rnw as per request by reviewer in [issue](Bioconductor#2 (review))

Rohit-Satyam added a commit to Rohit-Satyam/ggbio that referenced this pull request Jun 2, 2023

Update ggbio.Rmd

4901af0

Added space before and after the code block as suggested in Bioconductor/geneplotter#2 (comment)

Merge branch 'devel' into geneplotter-rmd

8eea13b

jwokaty approved these changes Jun 6, 2023

View reviewed changes

jwokaty merged commit 361d29f into Bioconductor:devel Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert byChroms.Rnw to byChroms.Rmd #2

Convert byChroms.Rnw to byChroms.Rmd #2

Rohit-Satyam commented Mar 15, 2023

jwokaty commented Mar 15, 2023

BerylKanali commented Mar 24, 2023

BerylKanali left a comment

jwokaty commented Mar 29, 2023

BerylKanali commented Mar 30, 2023

BerylKanali left a comment

BerylKanali Mar 31, 2023

Rohit-Satyam Apr 27, 2023

Rohit-Satyam commented Apr 1, 2023

BerylKanali commented Apr 1, 2023

mcarlsn commented Apr 13, 2023

Rohit-Satyam commented Apr 27, 2023 •

edited

Loading

Rohit-Satyam commented Apr 27, 2023

jwokaty left a comment

jwokaty May 6, 2023

Rohit-Satyam May 6, 2023

jwokaty May 6, 2023

Rohit-Satyam May 6, 2023

jwokaty May 6, 2023

Rohit-Satyam May 6, 2023

jwokaty May 6, 2023

Rohit-Satyam May 6, 2023 •

edited

Loading

jwokaty May 11, 2023

jwokaty May 6, 2023

Rohit-Satyam May 6, 2023

jwokaty May 11, 2023

Rohit-Satyam commented May 6, 2023

jwokaty left a comment

jwokaty May 14, 2023 •

edited

Loading

Rohit-Satyam Jun 2, 2023

Rohit-Satyam Jun 2, 2023

jwokaty Jun 2, 2023 •

edited

Loading

jwokaty commented Jun 6, 2023

		@@ -0,0 +1,90 @@
		---
		title: "geneplotter:Graphics related functions for Bioconductor"

		And finally we can test it by calling `cPlot`.
		```{r cPlot, fig=TRUE}

Convert byChroms.Rnw to byChroms.Rmd #2

Convert byChroms.Rnw to byChroms.Rmd #2

Conversation

Rohit-Satyam commented Mar 15, 2023

jwokaty commented Mar 15, 2023

BerylKanali commented Mar 24, 2023

BerylKanali left a comment

Choose a reason for hiding this comment

jwokaty commented Mar 29, 2023

BerylKanali commented Mar 30, 2023

BerylKanali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rohit-Satyam commented Apr 1, 2023

BerylKanali commented Apr 1, 2023

mcarlsn commented Apr 13, 2023

Rohit-Satyam commented Apr 27, 2023 • edited Loading

Rohit-Satyam commented Apr 27, 2023

jwokaty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rohit-Satyam May 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rohit-Satyam commented May 6, 2023

jwokaty left a comment

Choose a reason for hiding this comment

jwokaty May 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwokaty Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

jwokaty commented Jun 6, 2023

Rohit-Satyam commented Apr 27, 2023 •

edited

Loading

Rohit-Satyam May 6, 2023 •

edited

Loading

jwokaty May 14, 2023 •

edited

Loading

jwokaty Jun 2, 2023 •

edited

Loading