ecnuzdd commented 6 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/ecnuzdd/PhosMap

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 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.

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 help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

936ac31 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

7420ca9 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

351fc58 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

34fcca9 Update DESCRIPTION

ecnuzdd commented 5 years ago

Hello, I updeted version number in DESCRIPTION file and pushed a request via webhook trigger. But the operation did not work , because Github showed a prompt: "We couldn’t deliver this payload: Service Timeout" . Meanwhile, I do not receive a notice email. Please help me check whether my submission works. Many thanks.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

34fcca9 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

37b022e Update DESCRIPTION

bioc-issue-bot commented 5 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.

ecnuzdd commented 5 years ago

Hello, I have seriously check the build report. All compiling is ok except that there is only one warning when running "R CMD check".

The warnning prompts "R CMD check exceeded 10 min requirement", this is because PhosMap needs to load reference library when applying it to process Phosphoproteomics data. In our examples, we put these large files on a FTP server, once the example is tested, it first grasp reference data from FTP. The process would take a few minutes, which finally result in the warning from package builder of Biocoductor .

So, could the warning be ignored? If not, please give me some suggestion about fixing it.

ecnuzdd commented 5 years ago

@bioc-issue-bot Hello, I have seriously check the build report. All compiling is ok except that there is only one warning when running "R CMD check".

The warnning prompts "R CMD check exceeded 10 min requirement", this is because PhosMap needs to load reference library when applying it to process Phosphoproteomics data. In our examples, we put these large files on a FTP server, once the example is tested, it first grasp reference data from FTP. The process would take a few minutes, which finally result in the warning from package builder of Biocoductor .

So, could the warning be ignored? If not, please give me some suggestion about fixing it.

mtmorgan commented 5 years ago

Have you thought of using BiocFileCache to download the data only once? Or better, to make your data available on AnnotationHub or the related ExperimentHub so that it is there even after you lose interested, and is automatically cached locally?

ecnuzdd commented 5 years ago

@mtmorgan Thanks for your reply. Maybe I don't describle the question clearly.

My question is that "R CMD check" would test examples written in each Rscript, the smooth execution of these partial examples is dependent on some libraries with large sizes (> 200 MB), whose loading time will result in the warnning "R CMD check exceeded 10 min requirement". These loading process does not matter where the library file exists or no matter how to cache them.

Of course, when users perform PhosMap in practice, all dependence libraries will cache locally for reusing.

I am sure that all Rscript is OK, the only warnning at the moment is "R CMD check" exceeds time while it does not come from performability of Rscript.

mtmorgan commented 5 years ago

Revise the evaluated code so that it remains comprehensive but evaluates within the specified time limit. It is not satisfactory to assume that the R scripts are ok -- they may be now, but changes in your package or packages that your package depends on can break them in the future. Instead of running code on very large data, create an example data set that is enough to illustrate the analysis and to test your code.

The bottom line is that your package must build and check within specified time limits.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

11ba322 Update DESCRIPTION

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

1690823 update for R check cmd fd8b1cf Delete PhosMap.Rproj

bioc-issue-bot commented 5 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.

bioc-issue-bot commented 5 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.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

a4afa7b update_examples_for_R_CMD_check

bioc-issue-bot commented 5 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.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

c51e0b0 update_examples_for_R_CMD_check_01

bioc-issue-bot commented 5 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.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

e44271b update_examples_for_R_CMD_check_01

bioc-issue-bot commented 5 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.

bioc-issue-bot commented 5 years ago

Received a valid push; starting a build. Commits are:

152f691 Update DESCRIPTION

bioc-issue-bot commented 5 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.

mtmorgan commented 5 years ago

6 June, 2019

Sorry for my very slow review.

Generally, I'm concerned about the reliance on an ftp site for data in your package; past experience has been that the author does not maintain these sites in the long term, meaning that users cannot rely on package functionality. I'm also concerned about the efficiency of the code, especially the absence of vectorization and use of 'copy-and-append' as indicated below. Finally many of your examples do not evaluate because they have if(FALSE){}; this renders the examples useless and the examples should be revised so that they are fully evaluated, with only rare exceptions. Likely this means using small sample data included with the package, or data made available in ExperimentHub.

DESCRIPTION

good

NAMESPACE

Looks like this is a combination of roxygen and 'hand' generated; this is very confusing for those looking at your code. Use one style or the other.

NEWS

Make sure your formatting can be parsed by utils::news()

vignettes

All data used in your vignette must either be in your package, or in an ExperimentHub resource; it is not acceptable to download data from a private server or github
wrap lines carefully in code chunks so that they do not spill over page bounds on output
use fl <- tempfile(); dir.create(fl) rather than getwd(), ~/, or other directories in the user's file system.
Provide more narrative text to help the user understand the inputs and outputs of your code chunks.
Avoid large code chunks without intervening narrative text.

R (specific examples identified in the comments, but applicable throughout)

analysis_deps_anova.R:75; extract_psites_score.R:90 and many other places: avoid 'copy-and-append' pattern; instead, vectorize (best) or 'pre-allocate and fill', see http://bioconductor.org/developers/how-to/efficient-code/
analysis_deps_anova.R:33 use @import or @importFrom to make functions from other packages available in your package; avoid using requireNamespace().
use stop(), warning(), message() to signal error or informative messages to the user; use cat() only to display an object in a print() or show() method.
```
if(group_nlevels < 2){
  cat('\n', 'Not found pairwise comparison.', '\n')
  stop('')
}
```
should be
```
if(group_nlevels < 2)
  stop('Not found pairwise comparison.')
```
load_data_with_ftp.R:22 use BiocFileCache to manage data downloaded by the user
get_aligned_seq_for_mea.R:90 this code chunk looks like it could be vectorized, and could be re-written using efficient the effiicent Biostrings pacakge.

man

Wrap man page examples, including comment strings to 80 characters so that they display clearly in a standard console
Do not try to avoid evaluating example code with if (FALSE) {}, \dontrun{} or \donttest{}; if (FALSE){} is never appropriate; the latter are appropriate when a limited section of the example cannot be run by the user interactively (e.g., over-writing files) or during check (e.g., requiring user interaction).

ecnuzdd commented 5 years ago

Hi Martin，

Thanks for your careful review and constructive suggestion.

I will comply with these suggestion to perform point-by-point updating.

As for your concern about the reliance on an ftp site for data, I want to state that I specially apply for the ftp site from our network center to store demo data and library data of PhosMap and provide users with more smooth experience. I promise we will continuously maintain it and guarantee it works.

Your suggestion about R code is helpful for us, I will further standardize them.

The if(FALSE){} is an inappropriate format, I will replace them with \donttest{} for check.

Best regards!

Dongdong Zhan The Institute of Biomedical Sciences East China Normal University 500 Dongchuan Road Shanghai 200241, China On 6/7/2019 05:43，Martin Morgannotifications@github.com wrote： 6 June, 2019

Sorry for my very slow review.

Generally, I'm concerned about the reliance on an ftp site for data in your package; past experience has been that the author does not maintain these sites in the long term, meaning that users cannot rely on package functionality. I'm also concerned about the efficiency of the code, especially the absence of vectorization and use of 'copy-and-append' as indicated below. Finally many of your examples do not evaluate because they have if(FALSE){}; this renders the examples useless and the examples should be revised so that they are fully evaluated, with only rare exceptions. Likely this means using small sample data included with the package, or data made available in ExperimentHub.

DESCRIPTION

good

NAMESPACE

Looks like this is a combination of roxygen and 'hand' generated; this is very confusing for those looking at your code. Use one style or the other.

NEWS

Make sure your formatting can be parsed by utils::news()

vignettes

All data used in your vignette must either be in your package, or in an ExperimentHub resource; it is not acceptable to download data from a private server or github

wrap lines carefully in code chunks so that they do not spill over page bounds on output

use fl <- tempfile(); dir.create(fl) rather than getwd(), ~/, or other directories in the user's file system.

Provide more narrative text to help the user understand the inputs and outputs of your code chunks.

Avoid large code chunks without intervening narrative text.

R (specific examples identified in the comments, but applicable throughout)

analysis_deps_anova.R:75; extract_psites_score.R:90 and many other places: avoid 'copy-and-append' pattern; instead, vectorize (best) or 'pre-allocate and fill', see http://bioconductor.org/developers/how-to/efficient-code/

analysis_deps_anova.R:33 use @import or @importFrom to make functions from other packages available in your package; avoid using requireNamespace().

use stop(), warning(), message() to signal error or informative messages to the user; use cat() only to display an object in a print() or show() method.

if(group_nlevels < 2){ cat('\n', 'Not found pairwise comparison.', '\n') stop('') }

should be

if(group_nlevels < 2) stop('Not found pairwise comparison.')

load_data_with_ftp.R:22 use BiocFileCache to manage data downloaded by the user

get_aligned_seq_for_mea.R:90 this code chunk looks like it could be vectorized, and could be re-written using efficient the effiicent Biostrings pacakge.

man

Wrap man page examples, including comment strings to 80 characters so that they display clearly in a standard console

Do not try to avoid evaluating example code with if (FALSE) {}, \dontrun{} or \donttest{}; if (FALSE){} is never appropriate; the latter are appropriate when a limited section of the example cannot be run by the user interactively (e.g., over-writing files) or during check (e.g., requiring user interaction).

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

mtmorgan commented 5 years ago

If your package is ready for further review, please be sure to increment the version to trigger a build.

ecnuzdd commented 5 years ago

Hi Martin，

Thanks for your kind reminders.

We are updating PhosMap. Its new version will be coming soon.

Best regards!

Dongdong

mtmorgan commented 5 years ago

I'll mark this package as inactive. Comment here when you are ready to resume the review process.

bioc-issue-bot commented 5 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 interest in Bioconductor.

Bioconductor / Contributions

(inactive) PhosMap #935

6 June, 2019