scMET: Bayesian modelling of DNA methylation heterogeneity at single-cell resolution

andreaskapou commented 2 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/andreaskapou/scMET

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

Hi @andreaskapou

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: scMET
Type: Package
Title: Bayesian modelling of cell-to-cell DNA methylation heterogeneity
Version: 0.99.0
Authors@R: 
    person(given = "Andreas C.",
 family = "Kapourani",
 role = c("aut", "cre"),
 email = "kapouranis.andreas@gmail.com",
 comment = c(ORCID = "0000-0003-2303-1953"))
Description: High-throughput single-cell measurements of DNA methylomes can 
    quantify methylation heterogeneity and uncover its role in gene regulation. 
    However, technical limitations and sparse coverage can preclude this task. 
    scMET is a hierarchical Bayesian model which overcomes sparsity, 
    sharing information across cells and genomic features to robustly 
    quantify genuine biological heterogeneity. scMET can identify highly 
    variable features that drive epigenetic heterogeneity, and perform 
    differential methylation and variability analyses. We illustrate how 
    scMET facilitates the characterization of epigenetically distinct cell 
    populations and how it enables the formulation of novel hypotheses on the 
    epigenetic regulation of gene expression.
License: GPL-3 | file LICENSE
Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.1.2
Biarch: true
Depends: 
    R (>= 4.1.0)
Imports: 
    methods,
    Rcpp (>= 0.12.0),
    RcppParallel (>= 5.0.1),
    rstan (>= 2.18.1),
    rstantools (>= 2.1.0),
    VGAM,
    data.table,
    magrittr,
    MASS,
    logitnorm,
    ggplot2,
    matrixStats,
    assertthat,
    viridis,
    coda,
    BiocStyle,
    cowplot,
    stats
Suggests: 
    testthat,
    knitr,
    rmarkdown
LinkingTo: 
    BH (>= 1.66.0),
    Rcpp (>= 0.12.0),
    RcppEigen (>= 0.3.3.3.0),
    RcppParallel (>= 5.0.1),
    rstan (>= 2.18.1),
    StanHeaders (>= 2.18.0)
SystemRequirements: GNU make
biocViews: ImmunoOncology, 
    DNAMethylation,
    DifferentialMethylation,
      GeneExpression,
      GeneRegulation,
    Epigenetics,
      Genetics,
      Clustering,
      FeatureExtraction,
      Regression,
      Bayesian,
      Sequencing, 
      Coverage,
      SingleCell
VignetteBuilder: knitr

vjcitn commented 2 years ago

This is a very interesting submission.

suppressPackageStartupMessages(library(scMET))
suppressPackageStartupMessages(library(data.table))
suppressPackageStartupMessages(library(magrittr))
set.seed(123)

magrittr should not be needed insofar as R 4.1 includes |>. Also you have a particular format for single cell data and do not use Bioconductor's SingleCellExperiment class. This will increase efforts for users to transform the customary representation to your format. Please introduce converters or adapters or add code to accommodate this representation.

andreaskapou commented 2 years ago

Hi,

Thanks for the suggestions! I added conversion function going from scmet to SCE objects and vice versa. I also added a section in the vignette file to highlight this functionality! Bumped package version and pushed it on Github.

Bestm CAK

andreaskapou commented 2 years ago

Hi any update on this? Should I do any more changes? Since the deadline for new Bioconductor packages is ending soon. Thanks again!

lshep commented 2 years ago

The deadline for packages in the queue to make the release is end of April so there is still time. We will look at the package shortly.

vjcitn commented 2 years ago

Is this from your code?

## Chain 1: ------------------------------------------------------------
## Chain 1: EXPERIMENTAL ALGORITHM:
## Chain 1:   This procedure has not been thoroughly tested and may be unstable
## Chain 1:   or buggy. The interface is subject to change.
## Chain 1: ------------------------------------------------------------
## Chain 1:

Do we want this at this stage?

andreaskapou commented 2 years ago

Hi, thanks for the update!

No, actually the message comes from rstan where my probabilistic model is built. They still consider their Variational inference procedure experimental (see here: https://mc-stan.org/rstan/reference/stanmodel-method-vb.html) and they show this message whenever the function is called. I am happy to keep this message showing to the user, until its removed from rstan.

Thanks.

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". 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/scMET 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: 197891ce22093bd85806aa615e955c2feb13ea3e

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: "WARNINGS, TIMEOUT". 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/scMET 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: 7c83e86ea30a7a4ddce974da0bb582d1ec3d5c6a

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

andreaskapou commented 2 years ago

Hi,

The package was built and there were no errors only a single warning from the BiocCheck build report, which I am not sure what it means:

* Checking Package Dependencies... Warning in system2(cmd, args, stdout = TRUE, stderr = FALSE, env = "R_DEFAULT_PACKAGES=NULL") : running command 'R_DEFAULT_PACKAGES=NULL '/Library/Frameworks/R.framework/Resources/bin/R' -q --vanilla --slave -f /Library/Frameworks/R.framework/Versions/4.2/Resources/library/BiocCheck/script/checkBadDeps.R --args "/Users/pkgbuild/packagebuilder/workers/jobs/2585/7c83e86ea30a7a4ddce974da0bb582d1ec3d5c6a/scMET_0.99.4.tar.gz" "/var/folders/7y/c__3_48d5y18p1_rhfs_4x9c0000gt/T//RtmpTggVSV/file26656999e55/lib" 2>/dev/null' had status 1

I saw this error appeared in #2488 and you mentioned this was a bug in BiocCheck and now has been fixed. Could you let me know how should I proceed?

Thanks in advance!

Best, CAK

LiNk-NY commented 2 years ago

Hi @andreaskapou

Yes, this has been fixed in Bioconductor/BiocCheck@output which will be merged soon. You can ignore it. FYI, R has replaced mentions of --slave for the --no-echo flag.

Pete @PeteHaitch will provide a review in due time. Thanks Pete!

Best, Marcel

PeteHaitch commented 2 years ago

Hi @andreaskapou,

Thank you for submitting scMet to Bioconductor. I'm excited to see tools for analysing single-cell methylation data!

That said, I don't think @vjcitn's initial concern about usage of and interoperability with the core Bioconductor class for storing single-cell data (SingleCellExperiment) is adequately addressed in the current version. Before accepting the package, I think that the converter functions need to be re-designed (discussed below).

Longer term, I think having the main scMET function(s) accept a SummarizedExperiment-derived class (e.g., SingleCellExperiment or BSseq from bsseq) would be highly desirable rather than requiring the user to do the interop as separate step(s). A SummarizedExperiment-derived class is a much more natural (and richer, better-supported, more-tested, etc.) data structure than scmet's ad hoc S3 classes. Perhaps it is necessary to ultimately create a data.table/data.frame representation of the data to pass to the rstan-based or ggplot2-based functionality (that's not clear to me right now), but I really think it would be a big improvement to keep the data.table/data.frame representation as the internal business of scMET (rather than exposing it to the user) and instead exposing a more Bioconductor-familiar interface based on SummarizedExperiment-derived objected to users.

For acceptance into Bioconductor, there are a number of Required points, as well as Recommended points, that I would ask you to first please address. Would you please provide line-by-line comments to my initial review so that I know what changes I'm looking for in my re-review.

Cheers, Pete

Required

[ ] Re-design of converters between the scmet format and SingleCellExperiment class.
- [ ] The use of a special value (-1) to identify CpGs with no coverage is (1) unnecessary and (2) destroys the sparsity of the resulting matrices. Instead, just store a 0 in the corresponding entries of the met and total matrices; CpGs with no coverage can then be identified by those with a 0 in the total entry and there is no loss of sparisty for either the met or total matrices.
- [ ] When constructing the SummarizedExperiment-derived representation of the data, I would strongly consider storing the covariates X as metadata on the features (CpGs), i.e. as metadata(rowData(sce)) rather than metadata(sce).
[ ] Do not use direct slot access with @; please use the accessor instead (e.g., instead of sce@metadata use metadata(sce)).
[ ] The use of R's S3 object oriented system seems to me a bit non-standard. Bioconductor prefers S4 but I don't think it's a strict requirement (see https://contributions.bioconductor.org/r-code.html?q=S4#class-design); pinging @vjcitn for clarification. Admittedly, I mostly use S4 (due to Bioconductor's extensive reliance on it), but I am familiar enough with S3 through everyday R usage and resources such as https://adv-r.hadley.nz/s3.html to notice some things that seem a bit odd and might be refined:
- In several please you are creating ad hoc S3 classes (e.g., bb_mle from bb_mle(), scmet_mcmc and scmet_vb from scmet(), scmet_simulate and scmet_simulate_diff from scmet_simulate(), and scmet_differential from scmet_differential()) but there are no methods defined for these classes (E.g., methods(class = "bb_mle") returns no methods found).
- Instead, within a general function (e.g., scmet_plot_efdr_efnr_grid()) you're using if-else statements based on the class of the object to perform the required functionality.
- In S3, this would usually be achieved by defining an S3 generic and associated S3 methods, e.g., the generic scmet_plot_efdr_efnr_grid() and associated methods scmet_plot_efdr_efnr_grid.scmet_mcmc(), scmet_plot_efdr_efnr_grid.scmet_vb(), and scmet_plot_efdr_efnr_grid.scmet_differential().
- I believe inherits() is preferred to is() for S3 (https://adv-r.hadley.nz/s3.html#s3-classes and ?class).
[ ] Don't call set.seed() within a function. Instead, document to the user that they will need to set.seed() prior to calling the relevant function in order to get reproducible results.
[ ] Pinging @vjcitn: I'm not sure what to make of several of the scMET functions having a seed argument due to their internal use of rstan::sampling(); it seems that this is just the done thing with rstan (rather than using R's RNG) and any package using it just has to deal with it?
[ ] How many iter are required when running scmet()? Because setting it to 20,000 (as advised in vignette) still gave messages about the algorithm potentially not converging:

suppressPackageStartupMessages(library(scMET))
fit_obj <- scmet(Y = scmet_dt$Y, X = scmet_dt$X, L = 4,
                 iter = 20000, seed = 12)

# A whole bunch of output omitted ...

#> Chain 1: Informational Message: The maximum number of iterations is reached! The algorithm may not have converged.
#> Chain 1: This variational approximation is not guaranteed to be meaningful.
#> Chain 1: 
#> Chain 1: Drawing a sample of size 2000 from the approximate posterior... 
#> Chain 1: COMPLETED.
#> Warning: Pareto k diagnostic value is 3.14. Resampling is disabled. Decreasing
#> tol_rel_obj may help if variational algorithm has terminated prematurely.
#> Otherwise consider using sampling instead.
#> Wed Apr 20 16:13:58 2022 : Posterior inference finished.

[ ] Not clear why the outer plot() is required in plot(scmet_plot_estimated_vs_true(obj = fit_obj, sim_dt = scmet_dt, param = "gamma", mle_fit = bbmle_fit)) from the vignette; it wasn't when I ran the code myself in the console.
[ ] From the vignette: "Many thanks to John Riddell for the conversion functions.". John should be acknowledged as an author in the DESCRIPTION.
[ ] Please remove the LICENSE file (it is not required when using a 'standard' license like GPL-3).
[ ] inst/stan/include/license.stan seems redundant with the top-level license (i.e. License field in the DESCRIPTION).
[ ] inst/include/stan_meta_header.hpp is essentially empty. Is it required?
[ ] Please make an effort to address all WARNINGS and NOTES produced by BiocCheck::BiocCheck(). In particular, I would expect these to be remedied as advised (unless there are exceptional reasons):

* NOTE: 'LazyData:' in the 'DESCRIPTION' should be set to false or
      removed
* NOTE: Update R version dependency from 4.1.0 to 4.2.0.
* NOTE: Avoid sapply(); use vapply()
* NOTE: Avoid 'cat' and 'print' outside of 'show' methods
* NOTE: Avoid using '=' for assignment and use '<-' instead
* NOTE: Avoid redundant 'stop' and 'warn*' in signal conditions
* WARNING: Remove set.seed usage (found 2 times)
* NOTE: Consider adding runnable examples to the following man pages
  which document exported objects:
  create_design_matrix.Rd, sce_to_scmet.Rd, scmet_to_sce.Rd

[ ] Please upate the installation instructions in the README and vignette to the official Bioconductor installation instructions (can be delayed until after acceptance). These will be:

if (!require("BiocManager", quietly = TRUE))
    install.packages("BiocManager")

BiocManager::install("scMET")

[ ] It is strongly recommended that you add additional unit tests. Try running covr::report() on your package for ideas of where to start.
[ ] There are a 12 TODO comments in code. Are these issues that still need to be addressed or are these outdated?
[ ] Use \url{} in roxygen/.Rd markup to get clickable links in the docs.
[ ] I was initially a bit concerned by the potential need for a non-standard installation of V8. However, reading the links it sounds like its largely solved these-days? I'd consider de-emphasising this material, or noting that many (most?) users of scMet will not even need to worry about it.
[ ] Consider using R's native pipe operator, |>, instead of depending on magrittr for its %>% pipe operator.
[ ] Please run a spellcheck over the documentation and vignette.
[ ] Some of the data.table code would benefit from exposition, e.g. the following code from the vignette is pretty dense:

bbmle_fit <- scmet_dt$Y[, bb_mle(cbind(total_reads, met_reads))
                      [c("mu", "gamma")], by = c("Feature")]

[ ] Please clarify this comment in the vignette: "As we can see, scMET can more accurately estimate the overdispersion parameter γ by shrinking the MLE estimates towards to population average, resulting in more robust estimates." That's not clear to me from the figure (perhaps I'm misunderstanding of figure). E.g, there seem to be a good number of BB MLE points are closer to the diagonal than the scMET points.
[ ] Please correct language in vignette: "This will create a new slot named hvf within the fit_obj object."
- S3 classes don't have 'slots' (this is an S4-specific term, I believe; see https://adv-r.hadley.nz/s4.html).
[ ] Output of running scmet_differential() refers to 'features' in some places and 'genes' in others. Please clarify.
[ ] An application to real (rather than simulated) data would be a great addition to the vignette.
[ ] Please add a BugReports field to the DESCRIPTION (this is usually the issues page for the GitHub repo: https://github.com/andreaskapou/scMET/issues)
[ ] Please consider more selective imports using importFrom instead of import all with import in the NAMESPACE (probably most relevant for VGAM and perhaps rstantools; not advised for methods).
[ ] Consider updating inst/CITATION to refer to the Genome Biology publication.
[ ] The vignette should have an Introduction section (essentially the current Background section)
[ ] I don't believe the .datatable.aware in R/scMET-package is necessary
- Based on my reading of https://cran.r-project.org/web/packages/data.table/vignettes/datatable-importing.html, this isn't necessary because scMET lists data.table under Imports rather than Suggests.
- In its current form, you end up with a strange .datatable.aware printed under 'Usage' when a user does help("scMET-package").
[ ] When running R CMD check on scMET I get a note saying:

  * checking for GNU extensions in Makefiles ... NOTE
    GNU make is a SystemRequirements.

I don't understand this, because GNU make is already listed in SystemRequirements, but it seems like it might be another rstan-related thing (https://discourse.mc-stan.org/t/using-rstan-in-an-r-package-generates-r-cmd-check-notes/26628/3), so I think we can ignore it.

bioc-issue-bot commented 2 years ago

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

andreaskapou commented 2 years ago

Hi @PeteHaitch

Thanks for reviewing scMET and the detailed suggestions on making the package better. I agree that using S4 classes and making scMET accept SE derived objects would be optimal. However, in the current circumstances (my current post has shifted from DNA methylation) I do not have the time to refactor the whole package, but would leave it as future work?

Please see my line-by-line answers to your comments. Boxes with ticks are implemented as per your suggestions

Required

[x] Re-design of converters between the scmet format and SingleCellExperiment class.
- Thanks for the suggestions. All implemented now.
[x] Do not use direct slot access with @.
[x] S3 objects: The S3 objects were mostly created to have a certain structure in the generated outputs and was not intended to be 'true' objects, with getter and setter methods.
- Simplified the code to not require if-else statements for class, hence not requiring S3 generics anymore.
- Updated to inherits rather than is
[x] Don't call set.seed() within a function. Instead, document to the user that they will need to set.seed() prior to calling the relevant function in order to get reproducible results.
[x] How many iter are required when running scmet()
- This is a tricky one and really depends on the dataset. The STAN implementation of VB relies on black-box variational inference and potentially with relatively small sample sizes sometimes tends to 'search' around the local/global minima. We've seen that with larger sample sizes (e.g. working with real data), it tends to converge much faster, e.g. around 5k iterations.
[x] Not clear why the outer plot() is required. Removed outer plot.
[x] From the vignette: "Many thanks to John Riddell for the conversion functions.". John should be acknowledged as an author in the DESCRIPTION
[x] Please remove the LICENSE file (it is not required when using a 'standard' license like GPL-3).
[x] inst/stan/include/license.stan seems redundant with the top-level license (i.e. License field in the DESCRIPTION).
[x] Please make an effort to address all WARNINGS and NOTES.
- [] NOTE: 'LazyData:' in the 'DESCRIPTION' should be set to false or removed
  - Added this since I am exporting data (e.g. scmet_dt) and make them readily available when calling library(scMET).
  - Fixed most other NOTES, except the printing messages where I am using cat outside show methods.
[x] Please upate the installation instructions in the README and vignette to the official Bioconductor installation instructions (can be delayed until after acceptance).

[x] It is strongly recommended that you add additional unit tests. Try running covr::report() on your package for ideas of where to start.
- Additional tests will be added in the near future, as per your suggestion.
[x] There are a 12 TODO comments in code. Are these issues that still need to be addressed or are these outdated? They are outdated.
[x] Use \url{} in roxygen/.Rd markup to get clickable links in the docs.
[x] I was initially a bit concerned by the potential need for a non-standard installation of V8. However, reading the links it sounds like its largely solved these-days? I'd consider de-emphasising this material, or noting that many (most?) users of scMet will not even need to worry about it. --> added comment in README.
[x] Consider using R's native pipe operator, |>, instead of depending on magrittr for its %>% pipe operator.
[x] Please run a spellcheck over the documentation and vignette.
[x] Some of the data.table code would benefit from exposition
[x] Please clarify this comment in the vignette: "As we can see, scMET can more accurately estimate the overdispersion parameter γ by shrinking the MLE estimates towards to population average, resulting in more robust estimates." That's not clear to me from the figure (perhaps I'm misunderstanding of figure). E.g, there seem to be a good number of BB MLE points are closer to the diagonal than the scMET points.
- I agree that for some points MLE might be closer to the diagonal, but overall scMET results in better/ more accurate performance. Actual metrics are shown in the manuscript. One reason you might be seeing relatively large number of MLE points closer compared to scMET is that for efficiency we run scMET with small number of iter, hence the model not converging.
[x] Please correct language in vignette: "This will create a new slot named hvf within the fit_obj object."
[x] Output of running scmet_differential() refers to 'features' in some places and 'genes' in others. Please clarify.
[ ] An application to real (rather than simulated) data would be a great addition to the vignette.
- I agree and this will be added in the near future. A current issue we have is that scMET is relatively slow, hence building the vignette with real data would be prohibitive.
[x] Please add a BugReports field to the DESCRIPTION
[x] Please consider more selective imports using importFrom instead of import all with import in the NAMESPACE
[x] Consider updating inst/CITATION to refer to the Genome Biology publication.
[x] The vignette should have an Introduction section (essentially the current Background section)
[x] I don't believe the .datatable.aware in R/scMET-package is necessary

Best, Andreas

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

PeteHaitch commented 2 years ago

Thanks, Andreas. I've got grant deadlines this week but I will take a look next week.

Cheers, Pete

PeteHaitch commented 2 years ago

And once it's passing OK on BioC's build machines, please take a look at why it's now timing out there

andreaskapou commented 2 years ago

Hey Pete,

No worries at all and good luck with the grant application!

I increased the iter so I get a better fit in the vignette examples, which results in timing out. Will revert to old settings and submit again.

Best, CAK

bioc-issue-bot commented 2 years ago

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

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: "TIMEOUT". 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/scMET 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: ad34c91747183b3f0df570fdc326d49e6e6318b1

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: "TIMEOUT". 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/scMET 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: e89494a52f24b59cf3cc243e0c95832d43f314cf

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: "TIMEOUT". 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/scMET 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: d865ad36e1c13ad56cc3905c13e793c0952af423

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/scMET 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: 936c84af4decde40d0fa9d97c7e144a5487ee025

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

PeteHaitch commented 2 years ago

Thanks for your response to the review, @andreaskapou. The use of S3 is still non-standard, but since it doesn't seem to affect the package functionality and given your current circumstances feel free to leave as-is. A couple of final points to address, then we'll be ready to accept scMET. FYI I will be on leave until June 9, so there may be a delay in the package being formally accepted once you've made these changes.

Required

[ ] In scmet_to_sce() you create the sparse assays using e.g., total <- Matrix::Matrix(as.matrix(tot_Y), sparse = TRUE). This is going to be very inefficient because it first creates a dense matrix (as.matrix()) and then coerces that to a sparse matrix. Since you can figure out the row and column indices of the non-zero elements in the input (e.g., tot_Y), should be able to create the sparse matrix directly (and much more efficiently) through the sparseMatrix() constructor.
[ ] My apologies, I gave slightly incorrect advice in my initial review of, "When constructing the SummarizedExperiment-derived representation of the data, I would strongly consider storing the covariates X as metadata on the features (CpGs), i.e. as metadata(rowData(sce)) rather than metadata(sce)."
- In brief, use mcols(rowData(sce)) rather than metadata(rowData(sce)). You could then access, for example, the CpG density of the first 10 CpGs as rowData(sce)$cpg_density[1:10].
- In more detail, the metadata() accessor is intended for the global metadata of an object as a whole whereas the mcols() accessor is intended for the element-wise metadata of the individual elements of the object (see help("Vector-class", "S4Vectors")).
[ ] Use message() rather than cat() when providing informational messages; see https://contributions.bioconductor.org/r-code.html#end-user-messages (flagged by BiocCheck::BiocCheck("scMET_0.99.10.tar.gz"); the other NOTEs flagged by it have been explained or, based on my reading, can be treated as recommendations rather than requirements for scMET).

[ ] Suggest documenting your advice on iter in scmet(), i.e. words to the affect of what you wrote in response to my question: "This is a tricky one and really depends on the dataset. The STAN implementation of VB relies on black-box variational inference and potentially with relatively small sample sizes sometimes tends to 'search' around the local/global minima. We've seen that with larger sample sizes (e.g. working with real data), it tends to converge much faster, e.g. around 5k iterations.". Users will assume the default value is fine.

andreaskapou commented 2 years ago

Hi @PeteHaitch , please see my line-by-line answers to your comments. Boxes with ticks are implemented as per your suggestions.

[x] Create the sparse matrix directly (and much more efficiently) through the sparseMatrix() constructor.
- Thanks for this suggestions. Functions are updated accordingly.
[x] Use message() rather than cat() when providing informational messages: Done
[x] Suggest documenting your advice on iter in scmet(): Done
[ ] Use mcols(rowData(sce)) rather than metadata(rowData(sce)). You could then access, for example, the CpG density of the first 10 CpGs as rowData(sce)$cpg_density[1:10].
- Thanks for this suggestion, however I believe the current implementation might be better suited for storing covariates X. This is because X can have different number of columns and or names, depending on the analysis of the user. Hence it will be less error prone to consider X as an independent 'object', rather than deciding which columns to extract from mcols(rowData(sce)), and then regenerate the covariate matrix X. So I would be inclined to keep things as they are in terms of X, if that is OK with you?

Best, Andreas

bioc-issue-bot commented 2 years ago

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

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/scMET 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

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.

PeteHaitch commented 2 years ago

Thank you for making those changes, @andreaskapou. I've now accepted the package into Bioconductor.

(Apologies, I thought I had already done this a few weeks ago!)

PeteHaitch commented 2 years ago

@lshep or @Bioconductor/core: should my changing the tag to '3a. accepted' have automatically triggered the next steps ('The master branch of your GitHub repository has been added to Bioconductor's git repository.' etc.)?

nturaga commented 2 years ago

Hi Pete

Lori has to do a manual package ingestion for all the packages that have been accepted. I believe this happens once per week.

It should happen soon.

On Thu, Jul 14, 2022, 1:24 AM Peter Hickey @.***> wrote:

@lshep https://github.com/lshep or @Bioconductor/core https://github.com/orgs/Bioconductor/teams/core: should my changing the tag to '3a. accepted' have automatically triggered the next steps ('The master branch of your GitHub repository has been added to Bioconductor's git repository.' etc.)?

— Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/2585#issuecomment-1184002118, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU6QS5TZQQNLSYYRAOUPCTVT6QBHANCNFSM5ROXVGIA . You are receiving this because you are on a team that was mentioned.Message ID: @.***>

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

https://bioconductor.org/packages/scMET

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