Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

CDI #2297

Closed jfanglovestats closed 1 year ago

jfanglovestats commented 3 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 3 years ago

Hi @jfanglovestats

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: CDI
Version: 0.99.0
Date: 2021-07-06
Title: Clustering Deviation Index (CDI)
Description: Single-cell RNA-sequencing (scRNA-seq) is widely used to explore cellular variation. The analysis of scRNA-seq data often starts from clustering cells into subpopulations. This initial step has a high impact on downstream analyses, and hence it is important to be accurate. However, there have not been unsupervised metric designed for scRNA-seq to evaluate clustering performance. Hence, we propose clustering deviation index (CDI), an unsupervised metric based on the modeling of scRNA-seq UMI counts to evaluate clustering of cells.  
Authors@R: c(person("Jiyuan", "Fang", email = "jfanglovestats@gmail.com", role = c("cre", "aut")), person("Jichun", "Xie", email = "jichun.xie@duke.edu", role = c("ctb")))
biocViews: SingleCell, Software, Clustering, Visualization, Sequencing
URL: https://gitlab.oit.duke.edu/jichunxie/xie-lab-software_cdi
BugReports: https://gitlab.oit.duke.edu/jichunxie/xie-lab-software_cdi/-/issues
Imports: matrixStats, Seurat, stats, BiocParallel, ggplot2, reshape2, stringr, grDevices, ggsci
RoxygenNote: 7.1.1
Depends: R(>= 3.4)
Suggests: knitr, rmarkdown
VignetteBuilder: knitr
License: GPL-3
LazyData: true
Encoding: UTF-8
bioc-issue-bot commented 3 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 3 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/CDI to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 3 years ago

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

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

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

lazappi commented 3 years ago

Hi @jfanglovestats

Thanks for submitting CDI :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

General package development

DESCRIPTION file

NAMESPACE file

Package data

Documentation

Vignette

Man pages

Unit tests

Code

R

jfanglovestats commented 3 years ago

Hi Luke,

Thanks for your detailed comments. I will work on them soon and reply to you.

Best, Jiyuan

From: Luke Zappia @.> Reply-To: Bioconductor/Contributions @.> Date: Thursday, October 7, 2021 at 2:11 PM To: Bioconductor/Contributions @.> Cc: jfanglovestats @.>, Mention @.***> Subject: Re: [Bioconductor/Contributions] CDI (#2297)

Hi @jfanglovestatshttps://github.com/jfanglovestats

Thanks for submitting CDI 🎉! 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: 🚨 Required ⚠️ Recommended 🟢 Optional

General package development

DESCRIPTION file

NAMESPACE file

Package data

Documentation Vignette

Man pages

Unit tests

Code R

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/Bioconductor/Contributions/issues/2297#issuecomment-938035098, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE2C5LYZZYC2A5FAA35QOPLUFXPDXANCNFSM5EIMMWMQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

jfanglovestats commented 3 years ago

Hi Luke,

Thanks for the review of CDI package. I almost finish revising the package but do have one problem. You mentioned:

🚨 Rather than having an argument for the number of cores you should let users supply a BiocParallel::Param() object directly. This lets them have more control over how the parallelisation is done.

I changed my main function calculate_CDI to have one argument of bioc_parallel_obj:

calculate_CDI( sub_gcmat, cand_lab_df, cell_size_factor, bioc_parallel_obj = MulticoreParam(1), batch_label = NULL, lrt_pval_threshold = 0.01, clustering_method = NULL )

The problem is, if I restart R, only library(CDI) not library(BiocParallel), then the function only works if I don’t change the argument bioc_parallel_obj. If I try to change it to, for example, MulticoreParam(2), it shows

Error in h(simpleError(msg, call)) : error in evaluating the argument 'BPPARAM' in selecting a method for function 'bplapply': could not find function "MulticoreParam".

The BiocParallel is in DESCRIPTION Imports. Adding it to “Suggests” doesn’t help. Do you have any idea how to fix it?

Best, Jiyuan

From: Luke Zappia @.> Reply-To: Bioconductor/Contributions @.> Date: Thursday, October 7, 2021 at 2:11 PM To: Bioconductor/Contributions @.> Cc: jfanglovestats @.>, Mention @.***> Subject: Re: [Bioconductor/Contributions] CDI (#2297)

Hi @jfanglovestatshttps://github.com/jfanglovestats

Thanks for submitting CDI 🎉! 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: 🚨 Required ⚠️ Recommended 🟢 Optional

General package development

DESCRIPTION file

NAMESPACE file

Package data

Documentation Vignette

Man pages

Unit tests

Code R

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/Bioconductor/Contributions/issues/2297#issuecomment-938035098, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE2C5LYZZYC2A5FAA35QOPLUFXPDXANCNFSM5EIMMWMQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

lazappi commented 3 years ago

I think you probably need to prefix the function with the package name (eg. bioc_parallel_obj = BiocParallel::MulticoreParam(1)). A couple of other minor suggestions:

bioc-issue-bot commented 2 years ago

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

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

@lazappi The windows ERROR is on Bioconductor side. Please ignore this ERROR when re-reviewing the package since it does not appear on the other platforms.

jfanglovestats commented 2 years ago

Hi Luke,

Thanks a lot for your helpful comments and suggestions! We followed them to revise our package. Here we provide a point-to-point response document. Our responses are colored as blue under your comment. If the comment is addressed, we check the box in front of the comment.

Thanks!

Best, @jfanglovestats

Review

Key: 🚨 Required ⚠️ Recommended 🟢 Optional ☑️Revised in the code ✍️reply to the comments

General package development

lazappi commented 2 years ago

Hi @jfanglovestats. This is looking pretty good but I have a few more comments.

jfanglovestats commented 2 years ago

Hi @lazappihttps://github.com/lazappi,

Thanks a lot for your reply. May I ask some questions to clarify the comments?

  1. SingleCellExperiment object.

I tried to incorporate this type of object first. However, I met a problem when extracting the desired parts of the object. It seems SingleCellExperiment object has a flexible format, so different users could store the raw counts and gene names in different slots. For Seurat, it is quite standard to store the raw counts in @.**@. Do you have some suggestions to address this problem without using Seurat?

  1. MulticoreParam. Sorry I didn’t make it clear. Importing BiocParallel is necessary. What I am concerned is the following. Suppose CDI has a function argument PARAM taking the BiocParallel object. Suppose a user has installed CDI package in their computer but haven’t run library(BiocParallel). Then the following code will generate an error:

    Calculate_CDI(sub_gcmat = X, cand_lab_df = labs, BPPARAM = SerialParam())

    Even though CDI has imported BiocParallel, passing SerialParam() as an argument needs the user to install and library BiocParallel package. What I wrote in the current version does not need users to explicitly “library(BioParallel)”. Users can still pass all MulticoreParam arguments to the function. For MulticoreParam, I use it because from BiocParallel document, it is “the most efficient and least troublesome way to parallelize code. MulticoreParam uses ’forked’ processes with ’copy-on-change’ semantics – memory is only copied when it is changed.” When it is not available (like for Windows or when ncore = 1), MulticoreParam dispatches to Serial Param.

  2. Include R code for simulation Thanks for your suggestions! I was looking for a place to put the codes for simulation.

  3. Long lines

Thanks for your suggestions. I will try to make them shorter. I use long variable names, so it might be hard to make all lines short.

Best, Jiyuan

From: Luke Zappia @.> Reply-To: Bioconductor/Contributions @.> Date: Friday, December 10, 2021 at 10:36 AM To: Bioconductor/Contributions @.> Cc: jfanglovestats @.>, Mention @.***> Subject: Re: [Bioconductor/Contributions] CDI (#2297)

Hi @jfanglovestatshttps://github.com/jfanglovestats. This is looking pretty good but I have a few more comments.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/Bioconductor/Contributions/issues/2297#issuecomment-991072416, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE2C5L75I774CKV4LL6QNYLUQIM7PANCNFSM5EIMMWMQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

lazappi commented 2 years ago

I tried to incorporate this type of object first. However, I met a problem when extracting the desired parts of the object. It seems SingleCellExperiment object has a flexible format, so different users could store the raw counts and gene names in different slots. For Seurat, it is quite standard to store the raw counts in @.**@. Do you have some suggestions to address this problem without using Seurat?

Expression matrices will be stored in the assays slot of a SingleCellExperiment. These can be accessed with assay(sce, "assay_name"). There are also special accessors for when the assay is called "counts" (counts(sce)) or "logcounts" (logcounts(sce)). The best practice is to let the user select which assay they want to use by providing an argument to your functions. You will first need to import the relevant functions from the {SingleCellExperiment} or {SummarziedExperiment} packages. The gene/feature names can be accessed simply using rownames(sce).

Sorry I didn’t make it clear. Importing BiocParallel is necessary. What I am concerned is the following. Suppose CDI has a function argument PARAM taking the BiocParallel object. Suppose a user has installed CDI package in their computer but haven’t run library(BiocParallel). Then the following code will generate an error: Calculate_CDI(sub_gcmat = X, cand_lab_df = labs, BPPARAM = SerialParam()) Even though CDI has imported BiocParallel, passing SerialParam() as an argument needs the user to install and library BiocParallel package. What I wrote in the current version does not need users to explicitly “library(BioParallel)”. Users can still pass all MulticoreParam arguments to the function. For MulticoreParam, I use it because from BiocParallel document, it is “the most efficient and least troublesome way to parallelize code. MulticoreParam uses ’forked’ processes with ’copy-on-change’ semantics – memory is only copied when it is changed.” When it is not available (like for Windows or when ncore = 1), MulticoreParam dispatches to Serial Param.

To avoid this error you should add the package prefix with BiocParallel::, so something like:

 Calculate_CDI <- function(sub_gcmat, cand_lab_df, BPPARAM = BiocParallel::SerialParam()) {
     ...
 }

That will let the user use the function without having to run library(BiocParallel) first (as long as it is included as a dependency and installed).

Hope that clears things up a bit. Please ask if you have any further questions.

lshep commented 2 years ago

@jfanglovestats May we expect any updates soon? We like to see updates in a few weeks time to keep the submission processing moving forward.

jfanglovestats commented 2 years ago

Hi Ishep,

Thanks for checking. I will update this week.

Best, Jiyuan

From: lshep @.> Reply-To: Bioconductor/Contributions @.> Date: Friday, January 14, 2022 at 9:45 AM To: Bioconductor/Contributions @.> Cc: jfanglovestats @.>, Mention @.***> Subject: Re: [Bioconductor/Contributions] CDI (#2297)

@jfanglovestatshttps://github.com/jfanglovestats May we expect any updates soon? We like to see updates in a few weeks time to keep the submission processing moving forward.

— Reply to this email directly, view it on GitHubhttps://github.com/Bioconductor/Contributions/issues/2297#issuecomment-1013188143, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE2C5L74WTOETJMLYMYT7F3UWAZHLANCNFSM5EIMMWMQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you were mentioned.Message ID: @.***>

lshep commented 2 years ago

Please request this issue be reopened when you are ready with changes and to continue with the review process.

bioc-issue-bot commented 2 years ago

This issue is being closed because there has been no progress for an extended period of time. You may reopen the issue when you have the time to actively participate in the review / submission process. Please also keep in mind that a package accepted to Bioconductor requires a commitment on your part to ongoing maintenance.

Thank you for your interest in Bioconductor.

jfanglovestats commented 2 years ago

@lshep Hope you are doing well. Sorry for the delay in updates. I was distracted by other stuff. I have updated the package to my GitHub repository according to the comments. Could you reopen the issue CDI #2297 so that I can resubmit it to Bioconductor?

bioc-issue-bot commented 2 years ago

Dear @jfanglovestats ,

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.

bioc-issue-bot commented 2 years ago

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

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/CDI 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: 2c322afc6ca4df1a903c9bd490f84fd98847d427

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/CDI 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: 227c0ea9ee56336708c05f3f53c899c22117906f

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/CDI 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: 2a45189d954e01823249943dfc9ac81d866e30c1

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/CDI 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: 2dd35bbb6fccb2d3170168bd00ed2332fee78214

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/CDI 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: f72eaab7174bddba49787d79f07fc8cb30d6141f

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/CDI 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: 7e7a942fb822155cf995c4ffb61a5ee3343508a8

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/CDI 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: 4b96df44faffda179348f4cf2530c8416e19a518

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

jfanglovestats commented 2 years ago

Hi @lazappi ,

Sorry for the delay! Thanks for your comments. I have revised the package according.


  1. It is great to support Seurat objects but {Seurat} is not a Bioconductor package. To be compatible with the Bioconductor ecosystem you should really support SingleCellExperiment objects.

I added an R file ExtractInfo.R to extract counts, cell type labels, and batch labels from the SingleCellExperiment. For all exported functions, I added examples illustrating how to use the function if the input is a SingleCellExperiment object.

  1. I’m not sure I understand your argument. If you are already using MulticoreParam you should be depending on {BiocParallel} so users will already have it installed (I'm not sure how checks are currently passing without you importing {BiocParallel}). Letting the user supply the param object means they can choose exactly how parallel processing is done, not just be limited to MultiCoreParam.

Now the input of the main function calculate_CDI takes a BiocParallel object as an argument. Users can specify SerialParam(), MulticoreParam(), and so on. I also added an example in calculate_CDI for parallel computing.

  1. If your example dataset is simulated using R code it could be helpful to include the code itself in the documentation. Another way to do this is using a data-raw folder, see https://r-pkgs.org/data.html.

Now the R codes that simulate the data are in data-raw. I also mentioned this in the data description.

Now the three main export functions (size_factor, feature_gene_selection, and calculate_CDI) all have unit tests.

I went through the code and tidied as many functions as I could.

Other: There is a note in the Bioconductor build report which said “Avoid using '=' for assignment and use '<-' instead”. I tried to avoid this note and checked many times of all equal signs, but I didn’t identify additional “=“ for assignment.

lazappi commented 2 years ago

I added an R file ExtractInfo.R to extract counts, cell type labels, and batch labels from the SingleCellExperiment. For all exported functions, I added examples illustrating how to use the function if the input is a SingleCellExperiment object.

This is ok but it would be better if your functions took a SingleCellExperiment/Seurat directly and then you used these extraction functions internally. That way users could provide the objects they already have without having to mess around extracting things themselves. Using S3 dispatch could help with doing this for the two different objects.

You may want to check that your examples do properly cover both objects, I think the size_factor() example doesn't cover Seurat input.

The extraction function should give an error if something can't be found rather than returning a string.

Now the R codes that simulate the data are in data-raw. I also mentioned this in the data description.

It would make your examples much neater if you used this simulated data included in the package in your examples rather than generating new datasets as part of the example. It also means you can avoid issues that might come up with randomly generating data.

Now the three main export functions (size_factor, feature_gene_selection, and calculate_CDI) all have unit tests.

This is still fairly minimal, please think about other ways you can test these functions (checking what happens with unexpected input is always useful).

Other: There is a note in the Bioconductor build report which said “Avoid using '=' for assignment and use '<-' instead”. I tried to avoid this note and checked many times of all equal signs, but I didn’t identify additional “=“ for assignment.

Possibly it has found something which isn't actually an assignment, I would just keep an eye out and fix it if you notice it later.

bioc-issue-bot commented 2 years ago

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

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/CDI 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: ba5ee88b92a92410a56b19f83e6783015b2b986a

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/CDI 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: cfa6c5bf96e732673930f34bebcd71e320951e0e

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

lazappi commented 2 years ago

Hi @jfanglovestats. It looks like the build is running without any issue now which is great. Please comment with the changes you have made when you are ready for me to review the package again.

lshep commented 2 years ago

@jfanglovestats is this ready for a re-review? could you re-summarize the changes for @lazappi

jfanglovestats commented 2 years ago

Hi Ishep and Luke,

Sorry I just relocated and started a new job. Lots of things have been going on this month. I will try my best to wrap up within a week.

Best, Jiyuan

From: lshep @.> Date: Thursday, October 6, 2022 at 7:49 AM To: Bioconductor/Contributions @.> Cc: jfanglovestats @.>, Mention @.> Subject: Re: [Bioconductor/Contributions] CDI (#2297)

@jfanglovestatshttps://github.com/jfanglovestats is this ready for a re-review? could you re-summarize the changes for @lazappihttps://github.com/lazappi

— Reply to this email directly, view it on GitHubhttps://github.com/Bioconductor/Contributions/issues/2297#issuecomment-1269888011, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE2C5L77KWVVIZLME3HNGR3WB24FXANCNFSM5EIMMWMQ. You are receiving this because you were mentioned.Message ID: @.***>