Autotuner - Githubissues

crmclean commented 5 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/crmclean/Autotuner

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

Hi @crmclean

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: Autotuner
Type: Package
Title: Automated parameter selection for untargeted metabolomics data processing 
Version: 1.0.1
Author: Craig McLean 
Maintainer: Craig McLean <craigmclean23@gmail.com>
Description: This package is designed to help faciliate data processing in
    untargeted metabolomics. To do this, the algorithm contained within the     package performs statistical inference on raw data to come up with the
    best set of parameters to process the raw data. 
License: MIT + file LICENSE
Encoding: UTF-8
Depends: R (>= 3.5)
LazyData: true
RoxygenNote: 6.1.1
biocViews: MassSpectrometry, Metabolomics
Imports: 
    MSnbase,
    devtools,
    xcms,
    RColorBrewer,
    dplyr,
    rlang,
    plyr,
    mzR,
    assertthat,
    scales,
    methods,
    entropy,
    cluster,
    grDevices,
    graphics,
    stats,
    utils
Suggests: 
    testthat (>= 2.1.0),
    covr,
    knitr,
    rmarkdown,
    mmetspData
VignetteBuilder: knitr
BugReports: github.com/crmclean/AutoTuner/issues
URL: github.com/crmclean/AutoTuner/

Add SSH keys to your GitHub account. SSH keys will are used to control access to accepted Bioconductor packages. See these instructions to add SSH keys to your GitHub account.

bioc-issue-bot commented 5 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your repository will NOT trigger a new build.

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

crmclean commented 5 years ago

I think I fixed the issues issue causing the build failure. Is there a way I could rerun this check?

Thanks, Craig

Kayla-Morrell commented 5 years ago

Hello @crmclean - Be sure that your web hook is set up properly so that pushes to the master branch of your repo will trigger a new build. Also, since your package is pre-release the versioning should be 0.99.z and the z should increment whenever you are committing changes to the package. Without the version bump a new build won't be triggered.

Hope this helps, Kayla

crmclean commented 5 years ago

Hey @Kayla-Morrell - Thanks so much for your help. I've confirmed that the webhook is working and changed the release to 0.99.0 since it was the first release with this syntax. It should work now, right?

Kayla-Morrell commented 5 years ago

@crmclean - I'm not sure where you changed the version to 0.99.0 because the DESCRIPTION file version still shows 1.0.1.

crmclean commented 5 years ago

@Kayla-Morrell Ah! I see my mistake now. I updated it on GitHub, but not within the description. Thank you for your patience and for helping me understand my mistake. Just made the change within the description file and pushed the commit.

Kayla-Morrell commented 5 years ago

@crmclean - Perfect, I see the change now and the builder recognizes that a version bump is required. Try bumping the version up to 0.99.1, push the commit, and this should start a new build.

bioc-issue-bot commented 5 years ago

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

d60249b updating bioconductor build

crmclean commented 5 years ago

It worked! Thanks, @Kayla-Morrell!

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

bioc-issue-bot commented 5 years ago

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

36f8801 Fixed build issue w vignette

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

bioc-issue-bot commented 5 years ago

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

5da141f Fixing dependency issue

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

bioc-issue-bot commented 5 years ago

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

8a1ba07 updating Desc

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

bioc-issue-bot commented 5 years ago

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

db6ca62 Fixed example

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

bioc-issue-bot commented 5 years ago

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

9466adc Updated bioC version

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.

Kayla-Morrell commented 5 years ago

Hello @crmclean -

Thank you for submitting to Bioconductor. Please see the initial review of the package below. Comment back here with updates that have been made and when the package is ready for a re-review.

General package development

[ ] CLARIFICATION: What is the purpose of the 'codecov.yml' file?

DESCRIPTION

[ ] REQUIRED: 'LazyData:' should be set to false. Having this field be true can slow down the loading of packages with large data.
[ ] REQUIRED: The url for 'BugReports:' is incorrect, there seems to be a 'g' missing in 'github'.

NEWS

[ ] REQUIRED: Thank you for including a NEWS file, this is helpful for keeping track of changes from one version to the next. However, the formatting is not correct. Please see https://bioconductor.org/developers/package-guidelines/#news for help on correct format.

data

[ ] CLARIFICATION: Where are the files 'peak_difference' and 'peak_table' used from the 'data/' directory?

vignettes

[ ] REQUIRED: Be sure to add author information to the information in lines 1-8.
[ ] SUGGESTION: We strongly encourage the use of table of contents.
[ ] REQUIRED: The 'Installation' section should demonstrate to the user how to install the package from Bioconductor. This can be done by adding the following code with 'eval = FALSE':

if(!requireNamespace("BiocManager", quietly = TRUE))
    install.packages("BiocManager")
BiocManager::install("Autotuner")

Also, edit the text to reflect this.

[ ] CLARIFICATION: Line 36, 'mmetspData' is not a package. Should this read that the additional data comes from the package 'mtbls2'?
[ ] SUGGESTION: Line 150-156, currently each of the parameters do not get their own line in text. To render text so that each parameter is on a seperate line then add an extra line space in between each of them.
[ ] SUGGESTION: Line 170-174, is there any way to adjust the plot size? Right now when the vignette is rendered the plot is very small and it is hard to tell what is being shown. If this can't be done in the function then you might have to adjust the width and height within the {r} part of the code chunk.
[ ] SUGGESTION: Thank you for adding SessionInfo(), this is good practice. To distinguish it a bit from regular code, just add a heading to this section entitled 'Session Info'.
[ ] REQUIRED: The file 'autotuner-schematic-final.png' should not be included unless it is actually being used in the vignette (which currently it doesn't seem to be).

man pages

[ ] SUGGESTION: A general note, only functions that are exported need to have man pages. Any function that this documentented that is not exported can have the tag @keywords internal. This will remove the function from the package index and disables some of its automated tests. These functions may be of interest to other developers extending your package but not to most users.
[ ] SUGGESTION: A general note, when defining the parameters in the functions the '-' before the definition is not needed. When the man pages are rendered is currently looks like this:

Autotuner: - An Autotuner objected containing sample specific raw data.

But if you remove the '-' it would just be Autotuner: An Autotuner....

Autotuner-class

[ ] REQUIRED: The 'Source:' does not make sense. There is not 'data-raw' folder and no 'makeTestData.R' file.

EICparams

[ ] REQUIRED: Consider adding runnable examples to this man page since it documents exported objects.

mzdb

[ ] REQUIRED: The 'Source:' path does not make sense.

observedPeak

[ ] REQUIRED: The 'Source:' path does not make sense.

inst/script

[ ] REQUIRED: How is the file 'eicParamEsts' created? The file 'eicParamsEsts' is documented and used most of the time, but 'eicParamEsts' is used in the 'test_paramSummary' file. Please make sure this file is documented or change the test accordingly.

R code

[ ] REQUIRED: Avoid sapply(); use vapply() instead. Found in files:
- checkEICFunctions, line 172, 400, 401, and 488
- checkEICPeaks, line 106
- extract_peaks, line 136
- findTruePeaks, line 69, 71, 78, 120
- plot_signals, line 142
- returnParams, line 48
[ ] REQUIRED: Avoid 1:..., use seq_len() or seq_along() instead. Found in files:
- Autotuner-class, line 97 and 113
- checkBounds, line 90
- checkEICFunctions, line 48, 76, 109, 307, and 471
- dissectScans, line 29
- EICParams, line 75
- estimate_loess, line 24 and 37
- extract_peaks, line 33, 128, 139, 140, and 144
- find_noise, line 12
- findPeakWidth, line 73
- findTruePeaks, line 47
- peak_time_difference, line 30 and 187
- peakwidth_est, line 64, 80, and 84
- peakwidth_table, line 44
- plot_peaks, line 45, 47, and 56
- plot_signals, line 45, 83, and 143
- ThresholdingAlgo, line 48, 51, and 52
[ ] REQUIRED: Avoid the use of direct slot access with '@'. Accessor methods should be created and utilized instead.
[ ] REQUIRED: Use various apply functions instead of for loops were possible.
[ ] SUGGESTION: For formatting purposes, consider multiple of 4 spaces for line indents, there are 230 line that are not.

Best, Kayla

crmclean commented 5 years ago

Thank you so much for all the feedback, Kayla. I will get to work on implementing them.

bioc-issue-bot commented 5 years ago

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

cbc24cf removed codcove file 4511fc4 Updating bioC version

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:

3b51f38 version bump

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.

crmclean commented 5 years ago

Dear @Kayla-Morrell

Thank you for all your help. I've gone through your wonderful suggestions and have implemented changes to the package to meet the Bioconductor guidelines.

Thank you for submitting to Bioconductor. Please see the initial review of the package below. Comment back here with updates that have been made and when the package is ready for a re-review.

General package development

[ x] CLARIFICATION: What is the purpose of the 'codecov.yml' file?

I removed this file.

DESCRIPTION

[ x] REQUIRED: 'LazyData:' should be set to false. Having this field be true can slow down the loading of packages with large data.

Done!

[ x] REQUIRED: The url for 'BugReports:' is incorrect, there seems to be a 'g' missing in 'github'.

Fixed! Thanks for pointing this out!

NEWS

[ x] REQUIRED: Thank you for including a NEWS file, this is helpful for keeping track of changes from one version to the next. However, the formatting is not correct. Please see https://bioconductor.org/developers/package-guidelines/#news for help on correct format. data

I have updated the structure of the news file to match those within the link you provided.

[ x] CLARIFICATION: Where are the files 'peak_difference' and 'peak_table' used from the 'data/' directory?

I removed these files from the directory because they were not being used within the package.

vignettes

[ x] REQUIRED: Be sure to add author information to the information in lines 1-8.

Added this info.

[ x] SUGGESTION: We strongly encourage the use of table of contents.

Added this info.

[ x] REQUIRED: The 'Installation' section should demonstrate to the user how to install the package from Bioconductor. This can be done by adding the following code with 'eval = FALSE':

if(!requireNamespace("BiocManager", quietly = TRUE)) install.packages("BiocManager") BiocManager::install("Autotuner") Also, edit the text to reflect this.

Added this to the vignette.

[ x] CLARIFICATION: Line 36, 'mmetspData' is not a package. Should this read that the additional data comes from the package 'mtbls2'?

Switched package names.

[ x] SUGGESTION: Line 150-156, currently each of the parameters do not get their own line in text. To render text so that each parameter is on a seperate line then add an extra line space in between each of them.

Done!

[ x] SUGGESTION: Line 170-174, is there any way to adjust the plot size? Right now when the vignette is rendered the plot is very small and it is hard to tell what is being shown. If this can't be done in the function then you might have to adjust the width and height within the {r} part of the code chunk.

I changed the dimensions, and the change made the image more clear for me.

[ x] SUGGESTION: Thank you for adding SessionInfo(), this is good practice. To distinguish it a bit from regular code, just add a heading to this section entitled 'Session Info'.

Done!

[ x] REQUIRED: The file 'autotuner-schematic-final.png' should not be included unless it is actually being used in the vignette (which currently it doesn't seem to be).

Removed

man pages

[ x] SUGGESTION: A general note, only functions that are exported need to have man pages. Any function that this documentented that is not exported can have the tag @keywords internal. This will remove the function from the package index and disables some of its automated tests. These functions may be of interest to other developers extending your package but not to most users.
[ x] SUGGESTION: A general note, when defining the parameters in the functions the '-' before the definition is not needed. When the man pages are rendered is currently looks like this:

Autotuner: - An Autotuner objected containing sample specific raw data.

But if you remove the '-' it would just be Autotuner: An Autotuner....

Done!

Autotuner-class

[x ] REQUIRED: The 'Source:' does not make sense. There is not 'data-raw' folder and no 'makeTestData.R' file. EICparams
[ x] REQUIRED: Consider adding runnable examples to this man page since it documents exported objects. mzdb
[ x] REQUIRED: The 'Source:' path does not make sense. observedPeak
[ x] REQUIRED: The 'Source:' path does not make sense.

Fixed source path for all files.

inst/script

[ x] REQUIRED: How is the file 'eicParamEsts' created? The file 'eicParamsEsts' is documented and used most of the time, but 'eicParamEsts' is used in the 'test_paramSummary' file. Please make sure this file is documented or change the test accordingly.

Added documentation for this object.

R code

[ x] REQUIRED: Avoid sapply(); use vapply() instead. Found in files:
[ x] REQUIRED: Avoid 1:..., use seq_len() or seq_along() instead. Found in files:
[ x] REQUIRED: Avoid the use of direct slot access with '@'. Accessor methods should be created and utilized instead.
[ x] REQUIRED: Use various apply functions instead of for loops were possible.
[ x] SUGGESTION: For formatting purposes, consider multiple of 4 spaces for line indents, there are 230 line that are not.

I was careful to indent all my functions to increase the legibility of the content.

Kayla-Morrell commented 5 years ago

Hello @crmclean,

Thank you for all the work done on this package. I have looked over the changes made and have just a few additional requirements before accepting your package. See the secondary review below and comment back here when you are ready for me to look it over again.

vignette

[ ] REQUIRED: Thank you for adding in a table of contents, it can be really helpful to users. However, now it seems like you have 2 table of contents. When you added the code to metadata of the vignette you declared toc: yes which makes a table of contents and then you also created a table of contents in lines 27:41. You only need to have one or the other. My suggestion would be to remove lines 27:41 and just have the metadata code create the table of contents.

man pages

mzDb

[ ] REQUIRED: The path for the 'Source:' is still not correct. Please fix this.

inst/script

[ ] CLARIFICATION: I'm still confused with two data files in the directory 'inst/extdata'. What is the difference between 'eicParamEsts.rds' and 'eicParamsEsts.rds'? In the 'inst/script' file they look to be the same exact thing just with different names. If they are the same then 'eicParamEsts' should be removed since it is commented out and the test for 'test_paramSummary' should use the 'eicParamsEsts' file instead.

Best, Kayla

bioc-issue-bot commented 5 years ago

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

fa0e8e2 Bioc round 2

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.

crmclean commented 5 years ago

Dear @Kayla-Morrell

Thanks so much for all the helpful feedback. I've learned a ton through this process! I have gone back and corrected the issues you brought up during the second round of review. In regards to the inst/script/ clarification, I deleted the redundant data object.

Craig

Kayla-Morrell commented 5 years ago

@crmclean - Thank you for making the changes, everything looks good and I'm happy to accept the package!

Best, Kayla

bioc-issue-bot commented 5 years ago

Your package has been accepted. It will be added to the Bioconductor Git repository and nightly builds. Additional information will be posed to this issue in the next several days.

Thank you for contributing to Bioconductor!

mtmorgan commented 5 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/crmclean.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("Autotuner"). The package 'landing page' will be created at

https://bioconductor.org/packages/Autotuner

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

Autotuner #1189

General package development

DESCRIPTION

NEWS

data

vignettes

man pages

inst/script

R code

vignette

man pages

inst/script