jinhyunju commented 7 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/jinhyunju/picaplot

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 Subversion (optionally via GitHub).

For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

bioc-issue-bot commented 7 years ago

Hi @jinhyunju

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: picaplot
Type: Package
Title: An R package for analyzing gene expression data with Principal / Independent Component Analysis
Version: 0.99.0
Date: 2017-01-18
Author: Jin Hyun Ju
Maintainer: Jin Hyun Ju <jinhyun.ju@gmail.com>
Description: The functionalities included in the picaplot package provides the user with a simple workflow for the application of ICA and/or PCA to gene expression data, and for the analysis of the results. Key features of picaplot includes the visualization of individual components, comprehensive HTML report generation, association testing between components with measured variables, and unsupervised clustering of component coefficients. 
License: GPL-2
LazyData: TRUE
RoxygenNote: 6.0.1
Depends: R(>= 3.4)
Imports:
    MASS,
    stats,
    ggplot2,
    knitr,
    mclust,
    rmarkdown
VignetteBuilder: knitr
biocViews: GeneExpression, PrincipalComponent, Visualization, Clustering

bioc-issue-bot commented 7 years ago

Your package has been approved for building. Your package is now submitted to our queue.

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

@LiNk-NY will review your package but I note that it doesn't interoperate with Bioconductor, e.g., via use of SummarizedExperiment. Please update your package to accept SummarizedExperiment, and to use other Bioconductor packages and classes as appropriate.

lawremi commented 7 years ago

It's also kind of discouraging that report generation requires pandoc. It seems a major downside of markdown is that there are no widespread native viewers. Why not just use HTML directly?

bioc-issue-bot commented 7 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 following build report for more details:

http://bioconductor.org/spb_reports/picaplot_buildreport_20170310125731.html

jinhyunju commented 7 years ago

Hello, @mtmorgan Thanks for the quick feedback on the package. Just to clarify your point, are you asking for features in the functions that automatically recognizes objects of classes such as SummarizedExperiment? If that is what you are asking for I will implement that functionality as soon as possible. Thanks again!

jinhyunju commented 7 years ago

Dear @lawremi, Thank you for the feedback! My original thinking was that basing the report generation on knitr would integrate well with the current R community since it is widely used. At the moment I am not quite sure how I would directly implement this feature in HTML and would very much appreciate it if you could point me in the right direction. I will try to reflect your suggestion as soon as possible. Thank you!

bioc-issue-bot commented 7 years ago

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

9a2e0e8 Subscribed to bioc-devel mailing list to fix check...

lawremi commented 7 years ago

There are lots of examples of packages generating HTML reports from a template. For example, ShortRead. I think that uses Biobase::copySubtitute(). Fancier would be the brew or whisker packages.

bioc-issue-bot commented 7 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". 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 following build report for more details:

http://bioconductor.org/spb_reports/picaplot_buildreport_20170310132055.html

bioc-issue-bot commented 7 years ago

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

a44a62c SummarizedExperiment features added 9e37aeb merged version number

bioc-issue-bot commented 7 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". 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 following build report for more details:

http://bioconductor.org/spb_reports/picaplot_buildreport_20170310173044.html

jinhyunju commented 7 years ago

@mtmorgan I have added functionalities to the main functions run_ica, run_pca and covariate_association_check enabling SummarizedExperiment objects as direct input.

LiNk-NY commented 7 years ago

Hi Jin-hyun, @jinhyunju Please be aware that the Bioconductor naming conventions recommend against using underscores "_" in function names. Please use camelCase for naming functions. Also, please avoid creating/loading global variables inside your functions or non-standard evaluation (see the first note in R CMD check) In the latter case, you should be using the internal methods of ggplot2 to avoid this (e.g., aes_).

Regards, Marcel

bioc-issue-bot commented 7 years ago

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

ba6dcb1 Changing function names with underscores to camelC... 9562964 Merge branch 'bc_submit' "merging changes reflect...

bioc-issue-bot commented 7 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 following build report for more details:

http://bioconductor.org/spb_reports/picaplot_buildreport_20170315220338.html

jinhyunju commented 7 years ago

Hello Marcel, @LiNk-NY Thank you very much for your quick feedback. I have changed all function names containing underscores to camelCase names and addressed the NOTEs related to ggplot2 variables by using aes_string(). The updated 0.99.3 version of picaplot has been pushed to the master branch. Thank you again!

Best Regards,

Jin

jinhyunju commented 7 years ago

For the record, the ERROR label created during the automated build seems to be originating from a problem on the server related to the availability of "Biobase".

bioc-issue-bot commented 7 years ago

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

96f9a90 Version bump for a re-build

bioc-issue-bot commented 7 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "TIMEOUT". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/picaplot_buildreport_20170317124431.html

bioc-issue-bot commented 7 years ago

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

d83d44c Another version bump to check build

bioc-issue-bot commented 7 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 following build report for more details:

http://bioconductor.org/spb_reports/picaplot_buildreport_20170317181238.html

LiNk-NY commented 7 years ago

Hi Jin-hyun, @jinhyunju Thank you for submitting to Bioconductor. I have taken a look at your package and please find the review below. If you have any questions, feel free to respond here.

Best regards, Marcel

picaplot #316

DESCRIPTION

Move SummarizedExperiment to the Imports field

NAMESPACE

Looks clean and short :+1:

Vignette

It seems like the example in your vignette should include a SummarizedExperiment container rather than a matrix and a data.frame. This should be a serialized dataset that you can load for the example.
Avoid using _ for dataset names and use a more descriptive name for the dataset.
There's no need to have the create_example_data.R script in your package's inst folder. That can just be made into another vignette. But it does not seem like it uses the functions available in your package so it might be out of scope.

Grammar

Avoid the use of 1:n sequence generation as this is prone to unintended behavior for when n = 0
Avoid the use of class(x) == "class" language. If you are testing for S3 class membership, use inherits() or use is() for S4 classes.
Remove any messages that say Good. You only want to test for errors in the code not both.
You don't need to annotate error messages with Error, use the stop function.
You also don't need to use Caution in messages, use the warning function.
input_list should be an S4 class with validity checks often you are modifying the object but it seems like a more formal definition of the class would help
Please avoid using the sapply function for more robust code
For functions that are using a random seed, you want to have a seed argument so that the user can set a specific seed and make your code reproducible in the long run.
In the function icaRpar you may want to rename your fun argument into something that does not indicate a functional argument. Perhaps you want to use type, method, etc.
Don't use $ as R's interpreter uses fuzzy matching with this accessor, use the more robust way of element accession via [["elementName"]]
Establish default argument values instead of using NULL and changing it later.
Please be mindful of the argument order. In functions like runPCA, runICA, you want to have the SummarizedExperiment argument as the first one and the matrix and data.frame as second and third optional arguments.
Please create a helper function for all the "pheno" and "assay" checks that you repeatedly need to perform since you do allow those inputs for some functions. You can reuse this function for whenever you have a function with those arguments and avoid repetitive code.
When defining a class, you want to use the S4 system (e.g., pcaresult). You class names should not have `` in their names.
Avoid class assignment in the S3 way: e.g., class(x) <- "newClass"

Misc

Please consider @lawremi 's comments about using a direct HTML output rather than via rmarkdown.

jinhyunju commented 7 years ago

Dear Marcel (@LiNk-NY),

Thank you very much for your feedback and I apologize for the late response. I've recently started a new job which required a big move from the east coast to the west coast, so I had limited time on hand to work on your feedbacks.

I reflected most of the grammar related comments you have provided (bc_final branch on the picaplot repo) and am now starting the process to change the S3 system that is in place to the S4 system. However, since I am not familiar with the S4 system at all and this change needs broad code modification in almost every script, I think it will take me some time to get it done. Can you please confirm any deadlines or time restrictions if there are any?

Thank you again for your thorough feedback, and I will try my best to reflect your comments as soon as possible.

Best Regards,

Jin

LiNk-NY commented 7 years ago

Hi Jin-hyun, @jinhyunju There are currently no deadlines to submit the package. The deadline to submit the package to the upcoming Bioconductor release cycle has passed. Once you have made the changes and they have been accepted, your package will go into the development branch of Bioconductor. Best regards, Marcel

LiNk-NY commented 7 years ago

Hi Jin-hyun, @jinhyunju Any updates on the status of your package? Do you intend to work on it? Otherwise, I'd be forced to close the issue and possibly expect your resubmission.

Best regards, Marcel

jinhyunju commented 7 years ago

Hi @LiNk-NY, Apologies for the long radio silence. I've recently moved for a new job and had to devote most of my time in getting settled in (the reason why I asked you about the submission deadline). I think I will be able to work on this again starting the 2nd week of June. Thanks.

Best Regards,

Jin

jinhyunju commented 7 years ago

Hello @LiNk-NY,

I went through the changes that would be needed in order to reflect the feedback related to changing the current S3 implementation into S4 classes, and came to the conclusion that it would be almost equal to re-writing the whole package. It would be great if the current S3 structure can be kept, since I have another package depending on picaplot, which would be affected by the change. Would it be possible to submit the package on the current S3 implementation (reflecting feedback except S4 changes)? If you think the change to S4 is absolutely necessary, I would like to close the issue and stop the process for bioconductor submission. Thank you very much, and apologies for the delayed response.

Best Regards,

Jin

bioc-issue-bot commented 7 years ago

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

a88ed95 Fixed 1:n, sapply, NULL definition variables, and ... 0a12be8 removed two additional cases of 1:n fe4b2c9 Updated runICA documentation to address warning me...

LiNk-NY commented 7 years ago

Hi Jin-hyun, @jinhyunju

There are several reasons why we prefer using S4 classes instead of S3. These include but are not limited to robustness and validity checking. Your package would benefit from making classes such as input_list and others into S4 classes and avoid adding attributes to list-type classes.

Your argument about having other dependencies is not as strong as your submission is considered alpha at this stage but if you already have strong dependencies, I would have suggested that you first create a robust package.

Any thoughts on this @mtmorgan ?

PS. You can combine matrix and SummarizedExperiment type inputs into one argument and separate within the function (for convenience).

Regards, Marcel

bioc-issue-bot commented 7 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 following build report for more details:

http://bioconductor.org/spb_reports/picaplot_buildreport_20170628065547.html

LiNk-NY commented 7 years ago

Hi Jin-hyun @jinhyunju, Any updates on making changes to the package? There are some benefits that you can take advantage of by switching some of your data structures to an S4-based class.

Regards, Marcel

jinhyunju commented 7 years ago

Hi Marcel, @LiNk-NY Thanks for checking on the status. I agree that my package could benefit from adopting the S4 class structure in terms of robustness, however, I honestly don't think I have the bandwidth at the moment to work on such a large change. You can go ahead and close the ticket if necessary.

LiNk-NY commented 7 years ago

Hi Jin-hyun, @jinhyunju Do you know more or less when you will have time to work on the package? If it is a bit into the future, I can close it and you can either reopen it or create another issue.

Regards, Marcel

jinhyunju commented 7 years ago

@LiNk-NY , You can go ahead and close the ticket. Thank you very much.

LiNk-NY commented 7 years ago

Hi Jin-hyun, @jinhyunju Thank you for your submission. We hope to see your package with Bioconductor integration and with further development of your classes in S4 so that it benefits from validity checks and a structured definition. Best regards, Marcel

Bioconductor / Contributions

picaplot:package submission #316

picaplot #316

DESCRIPTION

NAMESPACE

Vignette

Grammar

Misc