adverSCarial - Githubissues

GhislainFievet commented 1 year ago

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

Repository: https://github.com/GhislainFievet/adverSCarial

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.

GhislainFievet commented 1 year ago

I have a general review related question. When using the bioconductor check function I tried to avoid 'NOTE' notifications, so I limited functions to 50 lines and lines to 80 characters. This makes my code quite difficult to read. Is this a point I should worry about? Can I let this go a little bit?

Thank you, best regards.

Ghislain

lshep commented 1 year ago

@GhislainFievet yes we can be a little lenient when it comes to these stylistic preferences. We put in a check as many would submit with no spacing, run-on lines, and massive functions that really should have been broken up for easy of maintenance. If it doesn't make sense for your code base and your code is readable to review you can ignore the warning.

GhislainFievet commented 1 year ago

ok thanks :-)

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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, skipped". 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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, skipped". 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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/adverSCarial to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

DarioS commented 1 year ago

Thanks for the updates. However, I notice there is one more code block that incrementally grows an empty vector.

lSplitsResults <- c(); i <- 1
while(i <= firstDichot) {
  lSplitsResults <- c(lSplitsResults, somethingElse)
 ...
 i <- i + 1
}

Also, operating on foundational classes such as SummarizedExperiment and SingleCellExperiment is a firm requirement for Bioconductor packages. @vjcitn, is only accepting a numeric matrix or data.frame of RNA-seq data O.K.?

' @param exprs a matrix or a data.frame of numeric RNA expression, cells are rows and genes are columns.

users should make their own functions to get the right matrix from the objects they use, SCE, Seurat, or raw data

Also, in the vignette, it shows how to import single cell RNA-seq data from a TSV file. I don't think single cell RNA-seq data would ever realistically be distributed as a TSV file. For example Human Cell Atlas Data Portal uses loom format. Simlarly, EMBL-EBI Single Cell Expression Atlas uses MatrixMarket format.

matPbmc <- system.file("extdata", "pbmc_short.tsv", package="adverSCarial")
matPbmc <- read.table(matPbmc, sep="\t")
cellTypes <- system.file("extdata", "cell_type_short.tsv", package="adverSCarial")
cellTypes <- read.table(cellTypes, sep="\t")$cell_type

It also recommends using Seurat objects which is generally incompatible with Bioconductor data containers.

Then we convert the test dataset into a Seurat object for visualization purpose.

Unexecuted code in third vignette gives warnings. The split warning of unequal lengths usually means incorrect calculations.

> adv_max_change <- advMaxChange(rna_matrix, clusters_id, "DC", CHETAHClassifier, advMethod="perc99", maxSplitSize = 2000)
genes size: 14405
current gene results length: 0
Modify data for 7203 genes for cluster DC
Preparing data....
Running analysis...
Warning messages:
1: In split.default(unlist(genes), c(1, 2)) :
  data length is not a multiple of split variable

Also, the result of this command, mat_max_adver, is very big:

    ...        ...
                   PTTG1IP ITGB2 ITGB2-AS1 LINC01547 FAM207A ADARB1 POFUT2 COL18A1 SLC19A1 PCBP3 COL6A1 COL6A2
                   SPATC1L LSS MCM3AP-AS1 MCM3AP YBEY C21orf58 PCNT DIP2A S100B PRMT2 ND1 ND2 COX1 COX2 ATP8 ATP6
                   COX3 ND3 ND4L ND4 ND5 ND6 CYTB LOC389831 LOC102724770 LOC107987373 LOC102724250
 [ reached getOption("max.print") -- omitted 4217 rows ]

You need to define a container class and show method for each kind of result, so the R command line isn't flooded.

The fourth vignette, which is all unevaluated code, doesn't work.

> seurat_markers <- FindMarkers(pbmc, ident.1 = "DC", ident.2="CD14+ Mono")
  Error in ValidateCellGroups(object = object, cells.1 = cells.1, cells.2 = cells.2,  :
  Cell group 1 is empty - no cells with identity class

Third and fourth vignettes (which use pre-determined results) need to be re-run with R version 4.3.0, now that it is the current version.
```
## R version 4.2.0 (2022-04-22)
## Platform: x86_64-pc-linux-gnu (64-bit)
## Running under: Ubuntu 20.04.4 LTS
```

GhislainFievet commented 1 year ago

Ok, thank you ! I will change that.

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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, skipped". 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/adverSCarial to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

lshep commented 1 year ago

@GhislainFievet all dependent packages must be on CRAN or Bioconductor. Please remove loomR.

GhislainFievet commented 1 year ago

@GhislainFievet all dependent packages must be on CRAN or Bioconductor. Please remove loomR.

Ok, thanks :-)

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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, skipped". 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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, skipped". 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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. 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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. 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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, skipped". 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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. 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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. 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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, skipped". 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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. 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/adverSCarial to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

GhislainFievet commented 1 year ago

Hello Darios,

here are the changes related to your last review:

I replaced the while loop by a lapply
I added the possibility to add a SingleCellExperiment in the functions, with a new argument argForClassif to specify which object needs to be given to the classifier
I converted my example single cell dataset as a loom file
I removed all use of Seurat functions and used BioConductors packages instead
the split warning was because I sometimes have to split uneven vectors, I coded a custom function to handle it without the warnings
I double checked the unevaluated chunks, everything should work fine, and faster
I upgraded to R 4.3

I have a concern about one point. Sometimes the results of my functions, avdMinChange and advMaxChange, can contain a lot of items, and you advised me to define specific class and a show method attached to them. I created two classes "advChar" and "advList", and specific show functions, which are declared on the .onLoad function. It works well, but it triggers a warning, I am a bit blocked on this. I understood that you can sometimes accept packages with warnings, could it be the case here? If it is not the case I would be very grateful to have an advice on how I could fix that :-)

Thanks again for your time.

Regards.

Ghislain

DarioS commented 1 year ago

Oh, please don't define classes in .onLoad. They should be defined in an ordinary R file, such as classes.R. Also, there is no documentation of the two new classes. Because the user will get an object of that class back from processing, the classes need to be documented. Please see how DESeqDataSet is implemented in the famous Bioconductor package DESeq2 for insipration. Also, thanks for switching to the Bioconductor package LoomExperiment for data management and increasing interoperability.

GhislainFievet commented 1 year ago

Ok thank you, I'm on it :-)

bioc-issue-bot commented 1 year ago

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

bioc-issue-bot commented 1 year 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. 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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. 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/adverSCarial 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 year ago

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

bioc-issue-bot commented 1 year 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. 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/adverSCarial to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

Bioconductor / Contributions

adverSCarial #2983

' @param exprs a matrix or a data.frame of numeric RNA expression, cells are rows and genes are columns.