AnneliesEmmaneel commented 4 years ago

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

Repository: https://github.com/saeyslab/PeacoQC

Confirm the following by editing each check box to '[x]'

[x] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
[x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
[x] I understand that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data. Note: [x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput cytometry data.
[x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.

I am familiar with the essential aspects of Bioconductor software management, including:

[x] The 'devel' branch for new packages and features.
[x] The stable 'release' branch, made available every six months, for bug fixes.
[x] Bioconductor version control using Git (optionally via GitHub).

For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

bioc-issue-bot commented 4 years ago

Hi @AnneliesEmmaneel

Thanks for submitting your package. We are taking a quick look at it and you will hear back from us soon.

The DESCRIPTION file for this package is:

Package: PeacoQC
Title: Peak-based selection of high quality cytometry data
Version: 0.99.3
Authors@R: 
    person(given = "Annelies",
 family = "Emmaneel",
 role = c("aut", "cre"),
 email = "annelies.emmaneel@hotmail.com")
Description: This is a package that includes pre-processing and quality control functions that can remove margin events, compensate and transform the data and that will use PeacoQCSignalStability for quality control. This last function will first detect peaks in each channel of the flowframe. It will remove anomalies based on the IsolationTree function and the MAD outlier detection method. This package can be used for both flow- and mass cytometry data.
Encoding: UTF-8
License: GPL (>=3)
LazyData: true
URL: http://github.com/saeyslab/PeacoQC
BugReports: http://github.com/saeyslab/PeacoQC/issues
Depends: R (>= 3.6)
Imports:
  circlize,
  ComplexHeatmap,
  flowCore,
  flowWorkspace,
  ggplot2,
  grDevices,
  grid,
  gridExtra,
  plyr,
  stats,
  utils
RoxygenNote: 7.0.2
Suggests: 
    knitr,
    rmarkdown
VignetteBuilder: knitr
biocViews: FlowCytometry, QualityControl, Preprocessing, PeakDetection

Add SSH keys to your GitHub account. SSH keys will are used to control access to accepted Bioconductor packages. See these instructions to add SSH keys to your GitHub account.

bioc-issue-bot commented 4 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your repository will NOT trigger a new build.

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

77928c3 Resolve warnings check

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

5bb806d Remove suppresswarnings and change MAD default 3eb8292 Merge branch 'master' of github.com:saeyslab/Peaco...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

2e204bd Update R dependence

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ABNORMAL". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

f6be43e Change dependancy

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

4becac9 Change dependancy to 4.0 f177b03 Merge branch 'master' of github.com:saeyslab/Peaco...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

nturaga commented 4 years ago

PeacoQC review

R CMD build

ok

R CMD check

ok

DESCRIPTION

ok

NAMESPACE

ok

man

ok

vignette

Consider using BiocStyle for the vignette. It adds formatting in a Bioconductor standardized way to you vignette. https://bioconductor.org/packages/release/bioc/html/BiocStyle.html
There are other ways to show commented out code, more stardard with how markdown is written. Please use eval=FALSE if needed to show commented out code.
The vigentte produces artifacts in the users directory. There should be clear warnings to show that this is happening. Usually vignettes when evaluated just produce an HTML file but do not save the plots and results.

Consider this, as a new user, there is a higher chance that the vignette is being read before the function man page.

R

You don't need which() in this line of code,

if (length(which(selection == FALSE))/length(selection) > 0.1)

You don't need paste0() in your warning() functions unless you are recycling vectors,

warning(paste0("More then ",
    round(length(which(selection == FALSE))/length(selection) * 100, 2),
    "% is considered as a margin event in file ",
    basename(ff@description$FILENAME), ". This should be verified."))

Do not accecss slots in a class with @ like in ff@description. Please write accessor functions. Pleaes read, http://bioconductor.org/developers/how-to/efficient-code/.

There are multiple places in your code base where this is done.
It seems like you need the matrixStats::colMedian() function here. You can avoid writing a loop here and just use the highly optimized package in matrixStats.

  channel_medians <- vapply(breaks,
            function(x){stats::median(ff@exprs[x,channel])},
            FUN.VALUE = numeric(1))

I highly recommend you consider breaking up the function PeacoQCSignalStability(). Try to write smaller functions, and stitch them together in the final function PeacoQCSignalStability(). The way it is written right now, makes it very prone to errors.

I want to stress on this on other functions as well. It is extremely hard for the reviewer to follow a 200-300 line function. Break them up. This is important as good coding practice. It helps you write functions where blocks of code are being repeated. Please go through the entire codebase once and make these changes.
- Instead of if(display_peaks == TRUE) you can just write if(display_peaks).
- Tailor your end user messages to fit this pattern, http://bioconductor.org/developers/how-to/coding-style/
```
End-User messages
```

message() communicates diagnostic messages (e.g., progress during lengthy computations) during code evaluation. warning() communicates unusual situations handled by your code. stop() indicates an error condition. cat() or print() are used only when displaying an object to the user, e.g., in a show method.



* Please go through your code base and fix the issues i've mentioned. Note that i've
  mentioned issues on specific lines of code, but these issues happen repeatedly in your
  codebase. Fix all of them. I highly encourage you go through the following documents 
  on the Bioconductor website and follow them as rigidly as possible,

  http://bioconductor.org/developers/how-to/coding-style/

  http://bioconductor.org/developers/how-to/efficient-code/

* Consider writing unit tests as well.

nturaga commented 4 years ago

Hi,

We usually give maintainers 14 days to respond to comments to know that they are actively working on the package.

Please take this as your 14 day notice to respond.

AnneliesEmmaneel commented 4 years ago

Hi,

I'm sorry that I did not respond yet. I am currently working on the package but it is taking a bit more time than I thought.

Kind regards.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

772e526 Change dependency a49795d Changed peak function + increased strictness for t... 0217f01 Modify PeacoQC function to make compensation and t... 19c9c82 Allow for better mass cytometry cleaning by adding... 61e3871 Add parameter suffix_fcs and improve code 1203cc0 Change plot function for event rate to not start f... 82c4f79 Make sure default parameters are the same as in si... 45bd16e Change some wrong descriptions and make default MA... e058af9 markernames + colors heatmap c437a3b markernames + colors heatmap 51e077c Merge branch 'Version_R_3_6' of github.com:saeysla... 2f905f5 Merge branch 'Version_R_3_6' of github.com:saeysla... 3cf71b5 Merge branch 'Version_R_3_6' of github.com:saeysla... a31e52e Merge branch 'Version_R_3_6' of github.com:saeysla... 956247f Merge branch 'Version_R_3_6' of github.com:saeysla... 2bbf539 Improve messages and errors e94649b Clean up 00cf4e5 Plot can also be made without time channel 3ac8609 Adapt warnings increasing and decreasing channels 5fabfdd Adapt warnings increasing and decreasing channels 5bdd0b2 Merge branch 'Version_R_3_6' of github.com:saeysla... 86af26a Change bandwith function 93f3289 Merge pull request #1 from saeyslab/Version_R_3_6 ...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

lshep commented 4 years ago

Is this ready for review? If so please comment with a summary of what was addressed

AnneliesEmmaneel commented 4 years ago

No not yet, there are still some issues that I have to address.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

4c1f286 Split up PeacoQCSignalStability and alter descript...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

nturaga commented 4 years ago

Please note that if your package is ready for review you need to let me know.

AnneliesEmmaneel commented 4 years ago

Dear, I am currently trying to splitting up my code into multiple functions to make it a bit more readable. But while doing this, I am coming across some errors or ways where I can still improve my code.

It is a bit of a slow job but I am certainly working on it :).

For now, I tried to address most of the comments but I should still check for them all when my code is sort of finalized. When it is ready, I will try to respond here with what kind of changes I did and how I tried to improve my package but for now there is still some work to do.

My apologies that it is taking so long but this is the first time I created a package and I still have a lot to learn.

Thank you for your patience!

Kind regards, Annelies Emmaneel

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

5069c72 Split up PlotPeacoQC in multiple functions Alter c...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

805099e Remove overview function and rename PeacoQCSignaSt...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

nturaga commented 4 years ago

Hi, please let me know when you are ready for another review.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

90a5de4 Imported "Original_ID" column to cleaned flowframe

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

AnneliesEmmaneel commented 4 years ago

Hi, I think the package is ready for another review. I made following changes according to your remarks:

vignette

Consider using BiocStyle for the vignette. It adds formatting in a Bioconductor standardized way to you vignette.

The BiocStyle is now used for the vignette.

There are other ways to show commented out code, more stardard with how markdown is written. Please use eval=FALSE if needed to show commented out code.

I made more blocks and used the eval = FALSE for the blocks that are not run.

The vignette produces artifacts in the users directory. There should be clear warnings to show that this is happening. Usually vignettes when evaluated just produce an HTML file but do not save the plots and results. Consider this, as a new user, there is a higher chance that the vignette is being read before the function man page.

Since the usage of the package requires to make a new folder, I added some warnings in the vignette. The package will be used in a cleaning process and the figures made are too large to display directly. Please let me know if the warnings are not clear enough.

R

You don't need which() in this line of code,
if (length(which(selection == FALSE))/length(selection) > 0.1) 

I tried to rerun this code without the which but this ended up with my two lengths being the same. I need the shorter version of the (which(selection) == F) to be able to calculate the percentages.

You don't need paste0() in your warning() functions unless you are recycling vectors,

warning(paste0("More then ",
    round(length(which(selection == FALSE))/length(selection) * 100, 2),
    "% is considered as a margin event in file ",
    basename(ff@description$FILENAME), ". This should be verified."))

I removed all the paste0 arguments in my warning statements.

Do not accecss slots in a class with @ like in ff@description. Please write accessor functions. Pleaes read, http://bioconductor.org/developers/how-to/efficient-code/. There are multiple places in your code base where this is done.

I changed all the @s by their flowCore:: functions.

It seems like you need the matrixStats::colMedian() function here. You can avoid writing a loop here and just use the highly optimized package in matrixStats.
  channel_medians <- vapply(breaks,
            function(x){stats::median(ff@exprs[x,channel])},
            FUN.VALUE = numeric(1))

I tried using this function but I always ended up using a loop since the breaks variable contains a list of overlapping lists for which the medians have to be calculated. If you have a more efficient way to solve this, please let me know.

I highly recommend you consider breaking up the function PeacoQCSignalStability(). Try to write smaller functions, and stitch them together in the final function PeacoQCSignalStability(). The way it is written right now, makes it very prone to errors. I want to stress on this on other functions as well. It is extremely hard for the reviewer to follow a 200-300 line function. Break them up. This is important as good coding practice. It helps you write functions where blocks of code are being repeated. Please go through the entire codebase once and make these changes.

I tried to split up the bigger functions (PeacoQC (previously PeacoQCSignalStability) and PlotPeacoQC).

Instead of if(display_peaks == TRUE) you can just write if(display_peaks).

I changed all the occurrences where this happened except on one spot where for some strange reason my function stopped working if I used this type of coding (In the IsolationTreeSD function).

Tailor your end user messages to fit this pattern, http://bioconductor.org/developers/how-to/coding-style/

End-User messages

message() communicates diagnostic messages (e.g., progress during lengthy computations) during code evaluation.
warning() communicates unusual situations handled by your code.
stop() indicates an error condition.
cat() or print() are used only when displaying an object to the user, e.g., in a show method.

I tried to adapt everything according to these guidelines.

Please go through your code base and fix the issues i've mentioned. Note that i've mentioned issues on specific lines of code, but these issues happen repeatedly in your codebase. Fix all of them. I highly encourage you go through the following documents on the Bioconductor website and follow them as rigidly as possible, http://bioconductor.org/developers/how-to/coding-style/ http://bioconductor.org/developers/how-to/efficient-code/

I tried to adapt as much as possible according to these rules but I did not change my variable and function names yet. This was a whole lot of work and I don't really see the added advantage here. I can change the function names but the variable names will take a really long time.

Consider writing unit tests as well.

I am trying to write these but I find them very hard. They are not included yet in the package but I am working on them.

I hope these changes are all ok.

Kind regards, Annelies Emmaneel

nturaga commented 4 years ago

Hi,

I've taken a look at your package again it looks sufficient. I'll accept the package once a new build is done and i've taken a look at it.

Please bump the version again once, I want to see if the warning you are seeing on the build report persists.

Best,

Nitesh

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

4293789 Removed ff error

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details.

AnneliesEmmaneel commented 4 years ago

Dear,

I thought I fixed the issue but I figured out that the warning can be traced back to the latest flowCore or flowWorkspace package. My colleague had the same warnings with the latest updates but I did not until I updated these packages.

With kind regards, Annelies Emmaneel

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

7421af6 Replace flowcore function to avoid .local deprecat...

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Received a valid push; starting a build. Commits are:

f2cfead max bins set to 200

bioc-issue-bot commented 4 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

bioc-issue-bot commented 4 years ago

Your package has been accepted. It will be added to the Bioconductor Git repository and nightly builds. Additional information will be posed to this issue in the next several days.

Thank you for contributing to Bioconductor!

mtmorgan commented 4 years ago

The master branch of your GitHub repository has been added to Bioconductor's git repository.

To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/AnneliesEmmaneel.keys is not empty), then no further steps are required. Otherwise, do the following:

See further instructions at

https://bioconductor.org/developers/how-to/git/

for working with this repository. See especially

https://bioconductor.org/developers/how-to/git/new-package-workflow/ https://bioconductor.org/developers/how-to/git/sync-existing-repositories/

to keep your GitHub and Bioconductor repositories in sync.

Your package will be included in the next nigthly 'devel' build (check-out from git at about 6 pm Eastern; build completion around 2pm Eastern the next day) at

https://bioconductor.org/checkResults/

(Builds sometimes fail, so ensure that the date stamps on the main landing page are consistent with the addition of your package). Once the package builds successfully, you package will be available for download in the 'Devel' version of Bioconductor using BiocManager::install("PeacoQC"). The package 'landing page' will be created at

https://bioconductor.org/packages/PeacoQC

If you have any questions, please contact the bioc-devel mailing list (https://stat.ethz.ch/mailman/listinfo/bioc-devel); this issue will not be monitored further.

Bioconductor / Contributions

PeacoQC #1348

PeacoQC review

R CMD build

R CMD check

DESCRIPTION

NAMESPACE

man

vignette

R

vignette

R