Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

ScreenR package submission #2555

Closed EmanuelSoda closed 2 years ago

EmanuelSoda commented 2 years ago

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

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

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

For questions/help about the submission process, including questions about the output of the automatic reports generated by the SPB (Single Package Builder), please use the #package-submission channel of our Community Slack. Follow the link on the home page of the Bioconductor website to sign up.

bioc-issue-bot commented 2 years ago

Hi @EmanuelSoda

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: ScreenR
Type: Package
Title: Package to Perform High Throughput Biological Screening
Version: 0.99.0
Authors@R: person("Emanuel Michele", "Soda", role = c("aut", "cre"),
        email = "emanuelsoda@gmail.com")
Description: The aim of ScreenR is to help in the analysis of 
   High Throughput Biological Screening using pooled shRNAs. Those
   type of screening uses RNA-seq expression data to find candidate 
   hits. ScreenR tries to combine together the power of software like
   edgeR with the semplicity of package like the metapackage 
   Tydiverse. Using ScreenR a pipeline able to find candidate hits can 
   be applied oon RNA-seq data moreover it integrates a wide range 
   of visualization in order to shows the results obtained.
License: MIT + file LICENSE
Encoding: UTF-8
RoxygenNote: 7.1.2
Imports:
    methods (>= 4.0),
    rlang (>= 0.4),
    stringr (>= 1.4),
    limma (>= 3.46),
    patchwork (>= 1.1),
    strex (>= 1.4.2),
    tibble (>= 3.1.6),
    scales (>= 1.1.1),
    ggvenn,
    purrr (>= 0.3.4),
    ggplot2 (>= 3.3),
    stats,
    tidyr (>= 1.2),
    magrittr (>= 1.0),
    dplyr (>= 1.0),
    edgeR (>= 3.32)
Suggests: 
    rmarkdown (>= 2.11),
    knitr (>= 1.37),
    testthat (>= 3.0.0),
    covr (>= 3.5)
Config/testthat/edition: 3
Depends: 
    R (>= 4.1.0)
VignetteBuilder: knitr
biocViews: Software, AssayDomain, GeneExpression
LazyData: true
LazyDataCompression: xz
bioc-issue-bot commented 2 years ago

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

IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. It is required to push a version bump to git.bioconductor.org to trigger a new build.

Bioconductor utilized your github ssh-keys for git.bioconductor.org access. To manage keys and future access you may want to active your Bioconductor Git Credentials Account

bioc-issue-bot commented 2 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, 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/ScreenR to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

lshep commented 2 years ago

Please correct the ERROR and trigger a new build report before a formal in-depth review will take place.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 56797d513c07787a09b441c5168555fed3935bde

lazappi commented 2 years ago

There is an issue with the build report being pushed to GitHub at the moment but you can view it here http://staging.bioconductor.org:8000/job/17270/. It looks like there are still some errors to address.

EmanuelSoda commented 2 years ago

Thank you for the answer, one of the error is "System Files found that should not be git tracked", but I already added those files to the .gitignore file. Do you have any clue why they are still tracked? Many Thanks

lshep commented 2 years ago

If they were already committed once you would have to git rm them and git commit for them to be removed?

EmanuelSoda commented 2 years ago

Ok, I'll try this way

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: d23c11ef2e29a4f0471918dfcb87dbe0e93d412a

lshep commented 2 years ago

We are still having issues with our windows builder preventing the build report from display (I'm actively investigating). You can check out the recent build reports here: http://staging.bioconductor.org:8000/jobs/1646/ .

EmanuelSoda commented 2 years ago

Is the building report now working correctly?

lshep commented 2 years ago

I have temporarily removed the windows builder while I debug it, so you will able to see a mac and linux build if you push a change now.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: dd6374ad7a82ddb289a4478f37aea923ed73295f

bioc-issue-bot commented 2 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/ScreenR to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 5f0382b0817abef012c865493517a1d3f9ef132c

bioc-issue-bot commented 2 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/ScreenR to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

EmanuelSoda commented 2 years ago

Sorry for my question. I have to do other things or I have just to wait for the review?

Many thanks

lazappi commented 2 years ago

Now that the build is passing please wait for a review. I will try to do it in the next few days.

lazappi commented 2 years ago

Hi @EmanuelSoda

Thanks for submitting ScreenR :tada:! Below is my review of your package. Please reply here if anything is unclear or needs any further explanation.

What next?

Please address the comments as best as you can. When you are ready for me to check the package again please reply to let me know with a summary of changes you have made or any other responses.

Luke

Review

Key: :rotating_light: Required :warning: Recommended :green_circle: Optional :question: Question

General package development

DESCRIPTION file

NAMESPACE file

The NEWS file

Package data

Documentation

Vignette

Man pages

Unit tests

Code

R

lshep commented 2 years ago

@EmanuelSoda may we expect updates soon? We like to see progress in a 3-4 week time frame to keep the review process moving.

EmanuelSoda commented 2 years ago

@lshep I'm sorry for the delay, there were crazy day at work. I'm working on it. I'll push the updated version as soon as possible.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 5e37b7267633063fdca0907c16de205747566329

EmanuelSoda commented 2 years ago

Sorry for my late replay, I have tried to address all the reviews comment. I have just few doubts/points left to ask:

❓is it necessary to rename the loaded data? At the moment it feels like unnecessary variables are being created

In order to use some of the ScreenR the name of the column of the count table has to be separated by “_”. The vignetta shows an example in which this is not the case and so it shows how to do that in order to enable ScreenR to work properly.

❓ Is RPMS the same as CPM (Counts Per Million)? If yes, please use this name which is more widely known.

Yes, sorry I have changed the name in CPM.

⚠️ I got a warning from the code which produces the Venn diagram, please check if there is an issue here I checked the warning unfortunately this does not depend on my code but is due to another package ScreenR uses to create the venn diagram. I have sent to the creator of the package a pull request in order to fix this bug but I haven’t received any answer yet. Anyway is just a warning due to wrong parsing but it doesn’t affect the result displayed.

🚨 Is it necessary to create a custom class or could you use a standard Bioconductor object such as SummarizedExperiment? ScreenR uses an own object that now has getter and setter methods.

🚨 Please check the format of the NEWS file is correct Sorry, what do you mean? Can you please tell me the correct format for the NEWS file? I have created this NEWS file using biocthis::use_bioc_news_md()

Thanks a lot Emanuel

bioc-issue-bot commented 2 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: "skipped, 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/ScreenR to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

EmanuelSoda commented 2 years ago

I have pushed again but the massage from the bot did not appeared

lshep commented 2 years ago

Did you include a version bump? It will only rebuild with a version bump and push to git.bioconductor.org

EmanuelSoda commented 2 years ago

Yes, you are right! My bad, I'm very new to this. Do you have any clue why on linux is giving me that error? Those function are not present anymore because are now in camel_case.

lshep commented 2 years ago

linux is case sensitive. So while you have changed to lowercase in the DESCRIPTION collate, you need to also rename your files in the R directory to match.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: bab7ead83029122728bb71e63c08aef36d70bc1b

bioc-issue-bot commented 2 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/ScreenR to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 46fb449155b7aa01d70dae3fa754b41352db0f5b

bioc-issue-bot commented 2 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/ScreenR to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

EmanuelSoda commented 2 years ago

Hi @lazappi when you have time you can take a look to the changes that I have made

lazappi commented 2 years ago

Hi @EmanuelSoda

Sorry, I missed this was now passing checks. I will try to take a look this week.

Luke

lazappi commented 2 years ago

❓is it necessary to rename the loaded data? At the moment it feels like unnecessary variables are being created

In order to use some of the ScreenR the name of the column of the count table has to be separated by “_”. The vignetta shows an example in which this is not the case and so it shows how to do that in order to enable ScreenR to work properly.

Ok. This should be mentioned in the text of the vignette so that it is clear to the user why this is done.

🚨 Is it necessary to create a custom class or could you use a standard Bioconductor object such as SummarizedExperiment? ScreenR uses an own object that now has getter and setter methods.

Could you explain why you feel it is necessary to have a custom object? It looks like most of the functionality requires converting to a DGEList anyway so maybe the package could be build around that instead? Resuing standard objects makes it much easier for users to combine Bioconductor packages in their workflows and reduces the maintenance burden for developers.

🚨 Please check the format of the NEWS file is correct Sorry, what do you mean? Can you please tell me the correct format for the NEWS file? I have created this NEWS file using biocthis::use_bioc_news_md()

Yeah, this looks fine. I can't remember why I commented on this originally.

Further comments

EmanuelSoda commented 2 years ago

Hi @lazappi thanks for the answer.
I have a doubts or questions Here some points:   Could you explain why you feel it is necessary to have a custom object? The idea at the base of ScreenR is to perform a DE analysis of shRNAs associated to genes in a tidy way to do this the ScreenR object contains all the information to perform this type of analysis and it try to take care of how the analysis is implemented in a low level (for example transforming the ScreenR object in a DGEList or using the data_table to perform operation or plot data).      You have defined setter and getter generics for your object but I couldn't find any implementation of these?  Sorry but I didn't quite get the question. I have defined generics in the generics.R file using the setGeneric() function and then I have used the setMethod() ( in the screenr-class.R file) to implement the methods associated to screenr_object the specific signature. Is this not enough? If not, could you please explain how to implement those functions?     Please add a table of contents to the vignette  I have used the following configuration in the YAML block but I don't know why it does not show the table of content. Do you have any suggestions on this?  

output: 
    BiocStyle::html_document:
    toc: true

  The dataset documentation still doesn't include a source. There should be a reference or at least a URL for where the data came from  The data comes from un still unpublished shRNAs synthetic lethality screening perform in my lab for this reason I cannot add a link right now. But basically, we followed the Cellecta protocol as reported in the web page. If is this really a problem a could try to find some already published data online.

lazappi commented 2 years ago

Hi @lazappi thanks for the answer. I have a doubts or questions Here some points:   Could you explain why you feel it is necessary to have a custom object? The idea at the base of ScreenR is to perform a DE analysis of shRNAs associated to genes in a tidy way to do this the ScreenR object contains all the information to perform this type of analysis and it try to take care of how the analysis is implemented in a low level (for example transforming the ScreenR object in a DGEList or using the data_table to perform operation or plot data).

I think you could probably have done that with an existing object but it should be ok if you think there is enough reason for a custom one. In the future it might be worth providing exported functions which allow the user to convert to some of the more standard formats.

You have defined setter and getter generics for your object but I couldn't find any implementation of these?  Sorry but I didn't quite get the question. I have defined generics in the generics.R file using the setGeneric() function and then I have used the setMethod() ( in the screenr-class.R file) to implement the methods associated to screenr_object the specific signature. Is this not enough? If not, could you please explain how to implement those functions?

Somehow I managed to miss these, I just had a quick look and they should be ok.

Please add a table of contents to the vignette  I have used the following configuration in the YAML block but I don't know why it does not show the table of content. Do you have any suggestions on this?  

output: 
    BiocStyle::html_document:
    toc: true

This should be fine. Obviously I wasn't really awake when I checked this, sorry about that.

  The dataset documentation still doesn't include a source. There should be a reference or at least a URL for where the data came from  The data comes from un still unpublished shRNAs synthetic lethality screening perform in my lab for this reason I cannot add a link right now. But basically, we followed the Cellecta protocol as reported in the web page. If is this really a problem a could try to find some already published data online.

I will leave this up to you. I think ideally the data should be public so that users can see all the details if they are interested (also that means you don't have to share your unpublished data). If you do keep what you have currently then I think you need to add some more detail about what the data is (another reason to use public data, you can just point to the original description). Let me know what you decide to do but I think this is probably the last thing to sort out.

bioc-issue-bot commented 2 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 8d08c400718d1fa01c209e4b0d137fa50169a82c

bioc-issue-bot commented 2 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/ScreenR to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

EmanuelSoda commented 2 years ago

Hi @lazappi, now should be all good. Let me know If there are other issue to address.

Thanks Emanuel

lazappi commented 2 years ago

Hi @EmanuelSoda. I am happy with the dataset documentation now so I'm going to approve this. Congratulations for getting the package into Bioconductor 🎉! It can take a couple of days to be picked up by the build system but then it should be part of Bioconductor devel.

bioc-issue-bot commented 2 years ago

Your package has been accepted. It will be added to the Bioconductor nightly builds.

Thank you for contributing to Bioconductor!

Reviewers for Bioconductor packages are volunteers from the Bioconductor community. If you are interested in becoming a Bioconductor package reviewer, please see Reviewers Expectations.

lshep commented 2 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/EmanuelSoda.keys is not empty), then no further steps are required. Otherwise, do the following:

  1. Add an SSH key to your github account
  2. Submit your SSH key to Bioconductor

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("ScreenR"). The package 'landing page' will be created at

https://bioconductor.org/packages/ScreenR

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.