geneClassifiers

rkuiper commented 8 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/rkuiper/geneClassifiers

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

Hi @rkuiper

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: geneClassifiers
Type: Package
Title: Application of gene classifiers
Version: 0.99.0
Date: 2016-10-2
Author: Rowan Kuiper
Maintainer: R.Kuiper <r.kuiper.emc@gmail.com>
Description: Application of gene classifiers to mas5 or gcrma normalized expression data.
License: GPL-2
Depends: Biobase
Imports: methods, utils, stats
biocViews: GeneExpression, BiomedicalInformatics, Classification, Survival, Microarray

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

bioc-issue-bot commented 8 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/geneClassifiers_buildreport_20161003194845.html

rkuiper commented 8 years ago

The error should be resolved.

bioc-issue-bot commented 8 years ago

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

2b43d2e Vesion 0.99.1 Version bumb

bioc-issue-bot commented 8 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/geneClassifiers_buildreport_20161004143223.html

LiNk-NY commented 8 years ago

Hi @rkuiper, Thank you for submitting to Bioconductor. Please find below your package review:

Bioconductor integration

It's good that you use ExpressionSet objects

Description

It doesn't look like you need Biobase in the Depends field
May want to more to the description of the package

Vignettes

Based on your vignettes, your API seems to be just a couple of functions with different arguments.

Namespace

If your user is going to be using just a couple of functions, there is no need export all the functions created.
Selectively export and import functions.

Grammar

Use of @ accessor is discouraged, your class should have it's own accessor methods.
Avoid the use of periods in function names and stick to Bioconductor camelCase conventions
Avoid using which as an argument as it is also a function.
Constructions like as.numeric(eventChain["allow.reweighted"])>0 can be simplified to if (eventChain[["allow.reweighted"]]). You shouldn't have to coerce data in your own structure.
Use of 1:length(number) can be rewritten as seq_len(number).
Avoid adhoc class assignment (i.e., class(object) <- "A").. use the S4 way to create classes.
Don't use any semi-colons (;) in the code. Not necessary.

Structures

The eventChain seems like it can be simplified to a DataFrame since you use similar variables across the different elements in the eventChain.

Please make the appropriate changes to your submission and bump the version for a rebuild in the SPB. Looking forward to your response.

Thanks for submitting to Bioconductor!

Regards, Marcel

bioc-issue-bot commented 8 years ago

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

22c6ef7 Version 0.99.2 In response to the Bioconductor re...

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

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

bioc-issue-bot commented 8 years ago

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

00fcdf7 Version 0.99.3 Version bump

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

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

rkuiper commented 8 years ago

Hi @LiNk-NY

Many thanks for your comments.

I made the appropriate changes did bump the version to 0.99.2 and 0.99.3. However it keeps hanging on

WARNING: y of x.y.z version should be even in release

As this is not yet a release version, I assume I can ignore this warning?

In response to your review to replace S3 classes with S4, I did restructure the code resulting in a more modular structure and better maintainable code. Further I more selectively performed the importing and exporting in the NAMESPACE file (Using roxygen2) and made the grammar adjustments.

I hope this meets the requirements.

Kind regards,

Rowan

LiNk-NY commented 8 years ago

Hi Rowan @rkuiper, Yes, please ignore the warning for now. I will look into your changes and approve the package soon.

Thank you, Marcel

rkuiper commented 8 years ago

Hi Marcel @LiNk-NY

Before you take a look, I want to let you know that I am planning an update of the vignette and the help pages in the coming days. The program code however will remain as it is.

Rowan

bioc-issue-bot commented 8 years ago

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

21e2be9 Version 0.99.4 Vignette update

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

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

rkuiper commented 8 years ago

Hi Marcel @LiNk-NY

In addition to the earlier adjustments, I have now also updated the vignettes.

Kind regards,

Rowan

bioc-issue-bot commented 8 years ago

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

ac523a6 Version 0.99.4 Bug repair (in the initializing me...

bioc-issue-bot commented 8 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/geneClassifiers_buildreport_20161118091134.html

rkuiper commented 7 years ago

Hi Marcel @LiNk-NY

I am planning to submit a paper in which I am referring to the geneClassifiers package. Can you indicate when I can expect your decision?

Kind regards,

Rowan

LiNk-NY commented 7 years ago

Hi Rowan, @rkuiper Thank you for making those changes. I took a quick look at the updates you provided.

Here are some additional comments:

Avoid the use of the @ to access slots. You should create accessor generics/methods instead.
Don't use initialize for creating your class. The Bioconductor way of doing this is to create a constructor function and handling any input there.
Your dataset names are long and clunky. It would be good if you could shorten them just a bit.
Your datasets are included as scripts. Perhaps you should include them as .Rda files. See devtools::use_data to include data objects easily. Remember to use LazyData: true in your DESCRIPTION.
I wonder if the above method will remove the need for having a .classifierHookList internal function and check. Seems like you're trying to load datasets without using data?
Consider adding unit tests.

Regards, Marcel

LiNk-NY commented 7 years ago

Hi Rowan, @rkuiper

If you plan on making the changes, you can provide a short link to your package in the publication.

https://bioconductor.org/packages/geneClassifiers

That link will work once the package has been approved and uploaded.

Regards, Marcel

bioc-issue-bot commented 7 years ago

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

05f1c31 Version 0.99.6 -the only places where the @ to ac...

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/geneClassifiers_buildreport_20161221091640.html

bioc-issue-bot commented 7 years ago

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

72d6e16 Version 0.99.7 Previous commit (v0.99.6) could no...

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/geneClassifiers_buildreport_20161221093942.html

rkuiper commented 7 years ago

Hi Marcel @LiNk-NY

I made the following changes according to your comments: -the only places where the @ to access slots are used are in getter/setter functions. -rewrote the class 'initialize' functions as constructors. -shortened the dataset names -replaced the datasets by a systemdata.rda file for internal namespace use only -replaced the checks during package loading by unit tests.

I want to keep modular structure of the .classifierHookList such that classifiers are easily added or deleted.

Regards, Rowan

LiNk-NY commented 7 years ago

Hi Rowan, @rkuiper I am still seeing a bunch of calls with @ accessors:

> grep -rn "[a-z]@" *
R/Class_ClassifierResults.R:78:        return(object@batchCorrection)
R/Class_ClassifierResults.R:89:        return(object@weightingType)
R/Class_ClassifierParameters.R:113:        return(object@hasTrainingData)
R/Class_ClassifierParameters.R:124:        return(object@eventChain)
<truncated>

You can create additional accessor functions or perhaps you may want to represent the data in a different structure that is easier to access (List type? Bioc class?).

I saw that you added datasets in the sysdata.rda file. Aren't these similar to those you have in the scripts? Do you need both?

Regards, Marcel

rkuiper commented 7 years ago

Hi Marcel, @LiNk-NY

Yes, that is correct. These @ calls are from within the accessor functions to access the slots. They have to be accessed somewhere? Is there another way? This is the same way as it is done in other Bioconductor packages (e.g. the AffyBatch class in the affy package).

A classifier is defined by its parameters which are given in the scripts. Optionally, one its parameters is a training dataset which is stored in sysdata.rda.

Regards, Rowan

LiNk-NY commented 7 years ago

@rkuiper

One way to avoid this would be to use a different class for representing your data but I don't have one in mind. You can leave it like this for now.

Do you need both datasets in separate places? Perhaps you can combine them, use one only, or put them all in either format.

Marcel

bioc-issue-bot commented 7 years ago

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

09acb6c Version 0.99.8 Instead of single classifier defin...

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: "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 following build report for more details:

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

rkuiper commented 7 years ago

Hi Marcel, @LiNk-NY

I merged the classifier scripts with the sysdata.rda file.

Further, a warning was added for all 3 build systems. According to the check script I have to add the dependency R >=3.4 to the Description file. A version which isn't even available on many Linux LTS systems. I currently have it set to R>=3.3. Do you know why this is necessary? I'd rather have a lower version e.g. R >=2.10.

Regards, Rowan

LiNk-NY commented 7 years ago

Hi Rowan @rkuiper, It is not "necessary" per se but using the development version of Bioconductor requires R version >= 3.4. You can keep the lower version but also be aware that this will be the R version your package will be used with once it is in Bioc-devel. Regards, Marcel

Update: Bioconductor maintains a standard of including build-able vignettes with each package. You don't have to tell the user to use the vignette function. Please remove the startup message. We would also recommend the user to use browseVignettes instead of vignette since many packages include more than one vignette.

bioc-issue-bot commented 7 years ago

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

680b509 Version 0.99.9 Removed the startup message 'refer...

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/geneClassifiers_buildreport_20170111160050.html

rkuiper commented 7 years ago

Hi Marcel, @LiNk-NY

The startup message is removed.

Regards,

Rowan

LiNk-NY commented 7 years ago

Hi @rkuiper, Thank you for submitting to Bioconductor. Your package has been accepted.

Best regards, Marcel

LiNk-NY commented 7 years ago

Note: Future improvements:

Please make sure you include R (>= 3.4.0) in the Depends field. Yes, there are some LTS systems with lower versions of R but you're submitting to Bioconductor where Bioc 3.5 (devel) uses R (>= 3.4.0). Perhaps you may want to create a github repository branch to handle those situations?

If you need to create a replacement method, use setReplaceMethod for that and not a generic with an assignment operator.

Please use goodpractice::gp("geneClassifiers") for more information on further improvements you can make to your package (see https://github.com/mangothecat/goodpractice).

Bioconductor / Contributions