ZheFrench commented 2 months ago

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

Repository: https://github.com/ZheFrench/BulkSignalR

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 Bioconductor Package Naming Policy and acknowledge Bioconductor may retain use of package name.
[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.
[ 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.
[x ] I am familiar with the Bioconductor code of conduct and agree to abide by it.

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 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 months ago

Hi @ZheFrench

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: BulkSignalR
Type: Package
Title: Infer Ligand-Receptor Interactions from bulk expression (transcriptomics/proteomics) data, or spatial transcriptomics
Version: 0.99.0
Authors@R: c(
    person("Jacques","Colinge",email="jacques.colinge@inserm.fr",role="aut",comment = c(ORCID = "0000-0003-2466-4824")),
    person("Jean-Philippe","Villemin",email="jpvillemin@gmail.com",role="cre",comment = c(ORCID = "0000-0002-1838-5880")))
Description: Inference of ligand-receptor (LR) interactions from bulk
    expression (transcriptomics/proteomics) data, or spatial transcriptomics.
    BulkSignalR bases its inferences
    on the LRdb database included in our other package, SingleCellSignalR
    available from Bioconductor. It relies on a statistical model that
    is specific to bulk data sets. Different visualization and data
    summary functions are proposed to help navigating prediction results.
URL: https://github.com/jcolinge/BulkSignalR
BugReports: https://github.com/jcolinge/BulkSignalR/issues
License: CeCILL | file LICENSE
Encoding: UTF-8
LazyData: FALSE
Depends: R (>= 4.4)
biocViews: Network, RNASeq, Software, Proteomics, Transcriptomics, NetworkInference, Spatial
Imports:
    Biobase,
    BiocFileCache,
    rappdirs,
    httr,
    DBI,
    RSQLite,
    cli,
    dplyr,
    rlang,
    jsonlite,
    matrixStats,
    methods,
    doParallel,
    glmnet,
    ggalluvial,
    ggplot2,
    gridExtra,
    grid,
    png,
    Rtsne,
    ggrepel,
    foreach,
    multtest,
    igraph,
    orthogene,
    stabledist,
    circlize (>= 0.4.14),
    ComplexHeatmap (>= 2.0.0),
    stats,
    scales,
    RANN
Suggests: 
    knitr,
    markdown,
    rmarkdown
RoxygenNote: 7.3.2
VignetteBuilder: knitr

ZheFrench commented 1 month ago

After the appearance of status "awaiting moderation" , we spotted some bugs in the vignette when calling bubblePlotPathwaysLR function. We made a push on our repository bumping version 0.99.0 to 0.99.1. We also corrected in description the URL and BugReports URL. We tried to open a new issue but it seems it's not possible still the "awaiting moderation status" is hilighted. We are very sorry for the incovenience. Don't hesitate to tell us if we can do anything to make your deployment easier.

Might important to mention, we have another package called SingleCellSignalR already packaged in bioconductor where we need to do an update. This update will be based on S4 class actually in BulkSignalR we are trying to submit today.

ZheFrench commented 1 month ago

I unfortunately closed the issue but it's still under "awaiting moderation". I am wondering if it's a normal process or if it's struggling with something...

lshep commented 1 month ago

Please don't close the issue. You can make any additional changes to your github repository during this pre-review stage. Once there is a messaged that it has been added to our git.bioconductor.org location, then you would have to update both. Cheers

ZheFrench commented 1 month ago

This issue is still awaiting moderation since one week. I know that Package reviewers are volunteer but I was wondering if there is still a chance to have a review for this package. Thank you.

lshep commented 1 month ago

I moderate packages once, maybe twice a week -- We were behind because of builder updates that were needed see https://stat.ethz.ch/pipermail/bioc-devel/2024-September/020583.html I'll likely get to it in the next day or two. If the build reports are clear it will be assigned a reviewer. We say reviewers loosely should have a review in under 2 weeks.

lshep commented 1 month ago

I'm concerned at the time this package is taking to build/check. I currently tried to run a R CMD build locally, it still has not completed after 20 minutes which would not pass the current Bioconductor standards for packages?

ZheFrench commented 1 month ago

Sorry to hear that, we missed that point. We have done a major review of the code in the last push version 0.99.2 to run the R CMD check around ~ 10 minutes. R CMD build run in less than ~ 10 minutes. It should be ok now. Could you try again, please ? Thank you for your feedback.

bioc-issue-bot commented 1 month ago

Your package has been added to git.bioconductor.org to continue the pre-review process. A build report will be posted shortly. Please fix any ERROR and WARNING in the build report before a reviewer is assigned or provide a justification on why you feel the ERROR or WARNING should be granted an exception.

IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. All changes should be pushed to git.bioconductor.org moving forward. It is required to push a version bump to git.bioconductor.org to trigger a new build report.

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 1 month ago

Dear Package contributor,

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

Your package has been built on the Bioconductor Single Package Builder.

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

Please see the build report for more details.

The following are build products from R CMD build on the Single Package Builder: Linux (Ubuntu 24.04.1 LTS): BulkSignalR_0.99.2.tar.gz

Links above 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/BulkSignalR to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 month ago

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

bioc-issue-bot commented 1 month ago

Dear Package contributor,

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

Your package has been built on the Bioconductor Single Package Builder.

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

Please see the build report for more details.

The following are build products from R CMD build on the Single Package Builder: Linux (Ubuntu 24.04.1 LTS): BulkSignalR_0.99.3.tar.gz

Links above 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/BulkSignalR to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 month ago

A reviewer has been assigned to your package for an indepth review. Please respond accordingly to any further comments from the reviewer.

ZheFrench commented 1 month ago

Dear reviewer, it might be important to mention, we have another package called SingleCellSignalR already packaged in bioconductor where we need to do an update. This update will be based on S4 class actually in BulkSignalR but first BulkSignalR need to be accepted on Bioconductor. We hope to be in time to correct the code if needed and be able to update this other package. Thanks

ZheFrench commented 1 month ago

Dear reviewer,
we would like to evaluate the time to refactor the code from our side, as from our understanding, the deadline for devel Bioconductor 3.20 packages to pass R CMD build and R CMD check will be October 25th. As we mentioned earlier, we have another package to update using the S4 classes actually in BulkSignalR. We're a little worried about being on time. King regards, Thanks

LiNk-NY commented 4 weeks ago

Hi @ZheFrench My apologies. We have been very delayed with our reviews due to the number of submissions. Your package is 8th in my queue. I will try to finish the review today. Best regards, Marcel

LiNk-NY commented 4 weeks ago

Hi @ZheFrench

Thank you for your submission. Please see the review below.

Best regards, Marcel

BulkSignalR #3554

Please make a stronger case for inclusion in Bioconductor. The main classes used in the package are data.frame and your S4 classes which do not inherit from Bioconductor infrastructure. Consider interoperability with Bioconductor matrix-type classes.
Note that S4 generics in the package must have an explicit list of arguments when created.
The examples should be runnable and not wrapped in if (FALSE)

DESCRIPTION

Looks good.
Consider using tools::R_user_dir instead of rappdirs for package related directory locations.

NAMESPACE

Consider using more descriptive names e.g., colA, colB, cmpName, stats, param etc. are quite vague.
Reconsider the use of Biobase::testBiocCConnection. It is an old function.

vignettes

Include package: BulkSignalR in the YAML front matter and remove the package version note in the Load packages section
Remove any commented code from the vignette (BulkSignalR-Differential.Rmd)
Only use one worker for the vignette. Package runs will be done simultaneously and only one core is allowed per package. Users can, of course, modify this setting.
Avoid copy-and-pasting pre-computed code into the vignette. It is preferred that your functions run in the vignette.
Do not use static images in the vignette. Use evaluated code to generate plots.
Would it be possible to interface with spatial class objects for the Spatial Transcriptomics section of the vignette? It may be a more natural workflow than reading the pre-processed example data for the example. You may also want to consider using VisiumIO to import data (disclaimer: I am the maintainer of VisiumIO)
Remove png and dev.off calls from the vignette and use only the functions in your package. Do not use static images. If you are using figures from a paper, those should be kept in a separate repository.

R

Do not use print in the example code for getComplexes. The user should not want to see the body of the function.
Avoid using get and use Sys.getenv for environment variables with defaults in getComplexes.
Do not use if (FALSE) in examples (BSRClusterComp-class.R).
Use more descriptive slot names for the BSRClusterComp class.
Avoid conditionally defining generics (e.g., colA, colA<-, etc.) and use setGeneric directly. Also indicate the full list of arguments in the generic so that both the method and generic signatures match.
Use your own accessor and setter functions in learnParameters rather than obj@SLOT.
The internal functions e.g., .getEmpiricalNull should not require an extensive call since those names are present in obj@param already. Your own internal code should be user friendly (in the learnParameters,BSRDataModel-method :).
Avoid writing files for the user by default (filename argument in learnParameters,BSRDataModel-method).
For the allocate and fill method, create a list of finite length, e.g., in BSRDataModel-class.R:
```
ligands <- vector("list", length(ligands(sig)))
```
Use lengths instead of vapply(x, length, integer(1))
Use the as method for S4 classes rather than as.BSRDataModelComp with setAs.
Avoid using a method to create a class. It is better to use a constructor function to generate a class e.g., defineClusterComp,BSRDataModelComp-method for creating a BSRClusterComp class; seen in intialInference,BSRDataModelComp-method, and others.
Consider separating the graphing functionality into its own package.
Avoid assign calls in the .onLoad function and simply set package constants within an R file, e.g.,
```
url <- "https://partage-dev.montp.inserm.fr:9192"
```
Avoid overly verbose package startup messages in .onAttach.

tests

Include unit tests to increase robustness.

ZheFrench commented 2 weeks ago

@LiNk-NY , we would like to thank you for this review.
Unfortunately, we will not be in time to achieve the expected changes.
Still, we will update the code for the next release in April.

We hope that you could continue to advise us, at least for the following few questions, and that the efforts following this review will be kept in mind for the next release.

Avoid using get and use Sys.getenv for environment variables with defaults in getComplexes

Could you explain why ? I remind myself that Sys.getenv was deprecetad in Check so I switched it to get that dit not complained anymore.

Avoid conditionally defining generics (e.g., colA, colA<-, etc.) and use setGeneric directly. Also indicate the full list of arguments in the generic so that both the method and generic signatures match.

We really had trouble to find a clean description on how to code using S4 classes. Could you share a link or an example of what you expect ?

Avoid assign calls in the .onLoad function and simply set package constants within an R file, e.g.,

Assign here is used to assign a value to a name in a specific environment . We use environment variables, because as mentioned before, we have another package that can use these environment variables later. We want to be able to modify them and that these modifications are seen by the other package.

Consider separating the graphing functionality into its own package.

This is something we really want to keep inside the package if it is not prohibited.

The examples should ke runnable and not wrapped in if (FALSE)

This is truly a difficult constraint. We took inspiration from another widely used package here. We could integrate some more dataset example in data but I think the size package of 5MB will be exceeded a little bit .Is there a relative tolerance for build/verify time (at least less than 20 minutes) but also for package size (5MB-10MB)? Or are these recommendations absolutely mandatory/strict ? We already set up a mechanism to download data from a remote server...

I ask these questions because when it comes to single cells and its derivatives, the data is increasingly heavy...How do others do it ? Our main function to compute the stats might take almost 10 minutes already...

Sorry for the naïve questions, and thank you again for your time.

Maybe this last part need to be adress to @lshep :

Now, how do we proceed to continue pushing code changes to bioconductor in order to be integrated in the next (april) release. When will it be possible to push again code in the 3.20 dev branch (if I understand the whole process..)
I imagine the code push in the 3.19 dev we have done will be deleted ? and this contribution board will be close also ? How will this happen ?

Thank you all !

ZheFrench commented 1 week ago

Is this board still active ? I mean, as our development is still active, can we consider the review feed back still in progress ? @lshep And so, can we continue to work with the same reviewer (if it's ok with him, even if we plan to submit for the next release). This has the advantage of giving us time for corrections and review ? @LiNk-NY

We have switch our development environment to Rdevel 4.4.2 and Bioconductor 3.20 in order to correct the code and push the revision before January in order to get a review as soon as possible in order to be ready in advance for the april release.

As we mentioned earlier, we have also another package to update (already in bioconductor) using the S4 classes actually in BulkSignalR. So we really need BulkSignalR to be "accepted" early to update smoothly this other package.

Finally , Also I was wondering if the source https://github.com/ZheFrench/BulkSignalR can be set to private now, until the publication, since that it's already "pushed" on bioconductor for review.

Many thanks, King regards

LiNk-NY commented 1 week ago

Hi @ZheFrench

Could you explain why ? I remind myself that Sys.getenv was deprecetad in Check so I switched it to get that dit not complained anymore.

Can you be more specific? Sys.getenv was not deprecated. There is a check in BiocCheck that avoids developers using Sys.setenv which is different.

This is not standard practice. You should not need to use get if all your variables are defined within the function environment and in that case, you simply call the object by name.

We really had trouble to find a clean description on how to code using S4 classes. Could you share a link or an example of what you expect ?

Unfortunately, there is no comprehensive book on S4 classes and methods. See Advanced R and the code base for SummarizedExperiment here for examples.

We use environment variables, because as mentioned before, we have another package that can use these environment variables later. We want to be able to modify them and that these modifications are seen by the other package.

If you are using environment variables (or even options), there should be no need to use assign. Only call Sys.getenv. The user will set the value of the environment variable on their systems.

This is something we really want to keep inside the package if it is not prohibited.

It is not prohibited but by adding plotting functionality, you're overloading the dependencies and potentially creating a fragile package. This is why we don't have plotting functions in computational or infrastructure packages.

This is truly a difficult constraint. We took inspiration from another widely used package here.

That is not a Bioconductor package. Add runnable examples in order to avoid bit rot, and stale examples. This ensures that your code is continually checked by CI and/or the BBS.

We could integrate some more dataset example in data but I think the size package of 5MB will be exceeded a little bit .Is there a relative tolerance for build/verify time (at least less than 20 minutes) but also for package size (5MB-10MB)? Or are these recommendations absolutely mandatory/strict ?

Your functions should be able to run with really small examples even if they're not biologically accurate. The point is to demonstrate the functionality of your package by providing example inputs and getting example outputs.

How do others do it ?

They provide simulation data or mini datasets.

We have switch our development environment to Rdevel 4.4.2 and Bioconductor 3.20

Make sure you use R-devel version 4.5.0.

can be set to private now, until the publication, since that it's already "pushed" on bioconductor for review.

Yes, you can do that. We will close the issue in the meantime.

Best regards, Marcel

ZheFrench commented 1 week ago

Yes, you can do that. We will close the issue in the meantime

So from my understanding this board will be closed... When will it be possible to to create a new issue and restart the process for reviewing ?

Thanks a lot for your answers !

LiNk-NY commented 1 week ago

Yes, we will close the issue. I think you should be able to re-open when ready.

bioc-issue-bot commented 1 week ago

Dear @ZheFrench ,

We have reopened the issue to continue the review process. Please remember to push a version bump to git.bioconductor.org to trigger a new build.

lshep commented 1 week ago

I'm sorry maybe this should not be reopened? @ZheFrench I'm confused at what you were asking on the bioc-devel mailing list? does this need to be re-opened now or are you taking time to work on it?

ZheFrench commented 1 week ago

I will take time to work on it (should be ready in approximately two weeks to make a new push). It's fine for me if you let it open, but I do understand if you need to close it until that day. Thanks for your answer.

Le jeu. 7 nov. 2024 à 13:59, lshep @.***> a écrit :

I'm sorry maybe this should not be reopened? @ZheFrench https://github.com/ZheFrench I'm confused at what you were asking on the bioc-devel mailing list? does this need to be re-opened now or are you taking time to work on it?

— Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/3554#issuecomment-2462178445, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADK2EGEMAV5XB7TQL53A5RLZ7NP23AVCNFSM6AAAAABOJTFOXKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRSGE3TQNBUGU . You are receiving this because you were mentioned.Message ID: @.***>

-- Jean-Philippe Villemin - Bioinformatics, PhD -

Cancer Bioinformatics and Systems Biology

Institut de Recherche en Cancérologie de Montpellier

Inserm U1194

@. @.>*

lshep commented 1 week ago

we will close for now. Please leave a message here when you are ready to proceed

Bioconductor / Contributions