search

Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

HPAanalyze #757

Closed anhtr closed 6 years ago

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

bioc-issue-bot commented 6 years ago

Hi @trannhatanh89

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: HPAanalyze
Type: Package
Title: Retrieve and analyze data from the Human Protein Atlas
Version: 0.99.0
Date: 2018-05-02
Authors@R: person("Anh Nhat", "Tran", email = "trannhatanh89@gmail.com",
        role = c("aut", "cre"))
Description: Provide functions for retrieving, exploratory analyzing and 
    visualizing the Human Protein Atlas data.
Depends: R (>= 3.4.0)
License: MIT + file LICENSE
RoxygenNote: 6.0.1
Imports: dplyr,
    XLConnect,
    ggplot2,
    readr,
    tibble,
    xml2,
    reshape2,
    tidyr,
    stats,
    utils
Suggests: knitr,
    rmarkdown,
    devtools
VignetteBuilder: knitr
LazyData: true
biocViews:
    Proteomics,
    CellBiology,
    Visualization,
    Software
bioc-issue-bot commented 6 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.

mtmorgan commented 6 years ago

@vobencha will review your package. It is not appropriate to use devtools::load_all() in a vignette; code must be in the vignette or R/ package directory.

Please ensure that your package returns and operates on standard Bioconductor objects such as SummarizedExperiment (for RNA-seq data), GenomicRanges, etc. Perhaps @lgatto or @jotsetung can provide proteomics guidance.

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

lawremi commented 6 years ago

For the sake of modularity, it would be nice to have a package that just retrieves the data, and another that analyzes data of the same type that is available from the atlas.

lgatto commented 6 years ago

It would be a good start to motivate the difference with hpar, and what the package offers more that manipulating HPA data.frames using standard R or dplyr functions.

anhtr commented 6 years ago

@mtmorgan Thank you for your comment. I am working on updating the vignette. I used devtools::loadall() in testing and forgot to update it. Regarding the standard Bioconductor objects, the majority of data from the atlas are summary of IHC/IF quantifications and I didn't think SummarizedExperiment would cover those. I have tried my best to use common R objects and did not create any new classes.

@lgatto: Thank you for your comment. This package was built to serve as a complementary package to hpar, and I have tried to have non-overlapping functionality. I am updating the vignette to clarify that, as well as provide more details on how HPAanalyze works.

anhtr commented 6 years ago

@lawremi Thank you for your comment. I was in a dilemma of having two straightforward packages doing data retrieval and analysis, or have one package that do both. I finally chose the second option as it will simplify the workflow and avoid confusion. I tried to write this package in a way that allow the user to ignore the data analysis part and just use the imported data as they see fit.

lgatto commented 6 years ago

@vobencha - just ping me if you want me to help with the review.

anhtr commented 6 years ago

Sorry, I did not mean to push the changes to GitHub. That's why I didn't bump the version. Good to know that Webhooks works, though.

vobencha commented 6 years ago

@lgatto a technical review from you would be great. I'll do a structure/format review. Thanks. Val

anhtr commented 6 years ago

I would like to ask for one or two days to have a major overhaul of the package. I took a look at previously declined/accepted packages, learning from them. It's my first time creating a package, so there have been mistakes, but I truly want to get them rights and have the package accepted. Thank everyone for your comments so far. :-)

lgatto commented 6 years ago

@trannhatanh89 - no worries. When you are happy with the result and the package builds/checks, ping me here and I'll start the review. In the meantime, don't hesitate to ask if you have any questions.

bioc-issue-bot commented 6 years ago

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

b2be0c5 0.99.1 fix .gitignore

anhtr commented 6 years ago

I bumped a version to see if my edits pass the checks or not. Functionality not completed yet.

bioc-issue-bot commented 6 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 6 years ago

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

c071bab camelCase for arguments eb258fd git rm --cached .Rproj.user 9616153 Fix camelCase arguments in vignettes a5d9af6 remove .Rproj.user/** from git tracking 76e0491 ... a0a1875 ... 1dc0b6d bump version to check for error, functionality unc...

bioc-issue-bot commented 6 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 6 years ago

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

86e9830 bump version. subscribed to mailing list

bioc-issue-bot commented 6 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 6 years ago

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

c5e48f8 0.99.4 Version bump to test for errors

bioc-issue-bot commented 6 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 6 years ago

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

c7f55e9 Update R version dependency 95b97d8 Update download.R documents 57890ba Update hpaXmlGet() 3a27ec0 0.99.5 Version bump to check for errors

bioc-issue-bot commented 6 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: "ABNORMAL". 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 6 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 6 years ago

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

a5cebb6 Update utils.R: seq_along() 6b8fee3 hpaXmlAntibody(), hpaXmlTissueExprSum() 0863b66 vignette update e07b338 hpaXmlTissueExpr() 3179956 0.99.6 Bump version to test builds

bioc-issue-bot commented 6 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 6 years ago

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

40e17e1 Speed up vignette 62a3eb3 0.99.7 Bump version to test build

bioc-issue-bot commented 6 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 6 years ago

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

a1d9ac6 Add README.md and NEWS 72092b5 Eval more chunks in vignette b19f4cf Partially update vignete 9fb5f77 ... 8685e6b Complete basic vignette outline b01490c Partially updated vignette, use BiocStyle 505ca37 0.99.8 Bump version for testing

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

bioc-issue-bot commented 6 years ago

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

a871938 Completed the downloadable data section 4412230 Update hpaVisSubcell show grid 90d1eb4 complete hpaVis section of vignette 2c43676 completed the hpaXml section of vignette 8fd313b completed the future developments section of vigne... 88d26d5 updated the hpaSubset section of vignette 079864d finished the first almost complete vignette 64d399a 0.99.9 Bump version to test build and review

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

anhtr commented 6 years ago

@lgatto I think the package is ready to be reviewed. I am looking forward to hear your comments. Thank you very much!

bioc-issue-bot commented 6 years ago

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

68178e0 update download.R coding style 240c9dc update utils.R coding style 5de110a update vis.R coding style a7f45d0 update xml.R coding style bf0c5f8 update vignette coding style e9b77a6 0.99.10 Updadted coding style

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

vobencha commented 6 years ago

Hi,

I've taken a first look at the package. I am most concerned about function reorganization (last comment).

1) Remove the ignore/ directory

2) \dontrun in man pages

There are a lot of \dontrun statments in your man pages. Each exported function should be evaluated at least once if at all possible. As you may know, the primary purpose of evaluated man pages examples are to test if everything is working as expected and to detect breakage due to a change in upstream dependencies etc.

~/repos/github/packageReview >grep -R dontrun HPAanalyze/
HPAanalyze/R/vis.R:#'   \dontrun{              
HPAanalyze/R/vis.R:#'   \dontrun{
HPAanalyze/R/vis.R:#'   \dontrun{
HPAanalyze/R/download.R:#'   \dontrun{
HPAanalyze/R/download.R:#'   \dontrun{
HPAanalyze/R/download.R:#'   \dontrun{
HPAanalyze/R/xml.R:#'   \dontrun{
HPAanalyze/R/xml.R:#'   \dontrun{
HPAanalyze/R/xml.R:#'   \dontrun{
HPAanalyze/R/xml.R:#'   \dontrun{
HPAanalyze/R/xml.R:#'   \dontrun{
HPAanalyze/man/hpaVisSubcell.Rd:  \dontrun{
HPAanalyze/man/hpaSubset.Rd:  \dontrun{
HPAanalyze/man/hpaDownload.Rd:  \dontrun{
HPAanalyze/man/hpaVisTissue.Rd:  \dontrun{              
HPAanalyze/man/hpaXmlGet.Rd:  \dontrun{
HPAanalyze/man/hpaXmlProtClass.Rd:  \dontrun{
HPAanalyze/man/hpaXmlTissueExpr.Rd:  \dontrun{
HPAanalyze/man/hpaXmlTissueExprSum.Rd:  \dontrun{
HPAanalyze/man/hpaXmlAntibody.Rd:  \dontrun{
HPAanalyze/man/hpaVisPatho.Rd:  \dontrun{
HPAanalyze/man/hpaListParam.Rd:  \dontrun{

These functions are of particular concern because they are not evaluated at all:

hpaXmlAntibody
hpaXmlTissueExprSum
hpaXmlTissueExpr
hpaXmlProtClass
hpaXmlGet

3) \value sections in man pages need more detail

For example, the hpaListParam man page

}
\value{
The output of this function is a list of vectors.
}

How long is this list? Does it have names? What type of data does it contain? Please revisit all man pages and add more detail to the \value section.

4) function organization

I think the functions are organized in a way that limits flexibility of future development. Instead of creating a new function for every disease/tissue I would recommend a single download or extract function, e.g.,

Have you thought of supporting a function that allows the user to browser the interface, i.e., list files available for download?

anhtr commented 6 years ago

Thank you, @vobencha , for your review and comments. I am glad that the issues you described are very reasonable and fixable. I will work on the package to address the concerns.

For the first three comments, I will continue to work to tidy up my codes and update the manuals. The reason why non of the hpaXml functions got a running example is because there was some conflicts with the tidyverse packages that kept throwing warning with R CMD check, only with the example in the man files. I have been working on fixing this issue by import individual functions instead of the whole package. Hopefully it will be fixed in the next commit.

As for the comment on function organization, I think it is a great idea to create the umbrella functions as you described. I will definitely work on those and get them ready for the next commit.

I have also thought of the supporting function to list available files for download. However, I haven't figured out a way for to get that data from HPA directly (so it will always be complete and up-to-date). I can certainly create an internal data frame for that, but it will only be updated with every version of Bioconductor.

I'm sorry for my delayed response. I have a rather unexpected dissertation coming up soon so I have not had enough time for coding.

vobencha commented 6 years ago

Are you planning to submit a new version? We have a policy to close issues if there has been no activity for 2 weeks.

anhtr commented 6 years ago

@vobencha Thank you for checking with me. I will submit a new version by the end of the week. I am sorry for the inconvenience.

bioc-issue-bot commented 6 years ago

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

90af5e5 minor edits ee7a305 tidy download.R with magarittr 7976c5f prevent masking warning in check() cd593c9 import less whole packages 4e94698 remove ignore folder from git tracking' cfb2542 remove ignore folder from git tracking 8e8ddeb import individual xml2 function and make hpaXml ex... 4f2e597 remove download.R /dontrun dd2cba1 remove vis.R /dontrun e5701bb remove unused column in sample data to reduce size 80c0de8 hpaVis(), hpaXml(), minor edits for performance 7d23859 Updated help file with details about output of eac... c3fa427 0.99.11 Bump version for build and review

bioc-issue-bot commented 6 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 6 years ago

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

245aaac is.character instead of == in hpaXml() 20b9a67 0.99.12 Bump version for build and review

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

anhtr commented 6 years ago

Hello @vobencha,

I have updated the package to address most of the concern that you raised in the last review. The changes are summarized in the NEWS file.

There was one universal function that you requested I decided not to include in this version: the GET function. Because the two kinds of data from HPA (downloadable datasets and XML files) are very different from each other, I think its more confusing to put them together. Instead, the hpaVis() and hpaXml() universal functions can get their own input data by default without the need for downloading data first.

Please review at your convenience and give me further comments. Thank you very much. Anh Tran.

vobencha commented 6 years ago

Hi, Thanks for making the changes.

New issues:

  1. hpaXml.Rd needs an example.

  2. hpa_downloaded_histology_v18.Rd

    • Add example showing how to load the data with data(hpa_downloaded_histology_v18). Show how to access the different tibbles.
    • State where the data were downloaded from (url).

  3. Why not re-name R/data.R to R/hpa_downloaded_histology_v18.R?

  4. hpaXml.Rd Add a /seealso section with links to the sub-extractor functions hpaXmlAntibody hpaXmlProtClass hpaXmlTissueExpr hpaXmlTissueExprSum.Rd

  5. hpaVis.Rd Add a /seealso section with links to the sub-extractor functions hpaVisPatho.Rd
    hpaVisSubcell.Rd hpaVisTissue.Rd hpaSubset.Rd

  6. I'd recommend combining hpaListParam() and hpaSubset() on one man page and hpaDownload() and hpaExport() on another. These are interrelated and it would be difficult for the user to find the complementary functions on separate man pages.

@lgatto we're ready for your technical review.

Valerie

anhtr commented 6 years ago

Thank you very much @vobencha. I am working to address the new issues.

bioc-issue-bot commented 6 years ago

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

3e610f1 Remove .Rproj ccf6549 rename data.R, fix hpaXml examples 05e39f0 update documentation for dataset b288931 combine hpaListParam() and hpaSubset() on one man ... e0923e7 0.99.13 Bump version for buid and review

bioc-issue-bot commented 6 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, 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 6 years ago

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

d1d974c fix one.R functions error (identical instead of ==... d527288 0.99.14 Bump version for build and review