search

Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

struct #1265

Closed grlloyd closed 4 years ago

grlloyd commented 5 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 5 years ago

Hi @grlloyd

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: struct
Type: Package
Title: Statistics in R Using Class Templates
Version: 0.3.0
Author: Dr Gavin Rhys Lloyd
Maintainer: Dr Gavin Rhys Lloyd <g.r.lloyd@bham.ac.uk>
Description: Defines a set of class templates for developing statistical workflows.
License: GPL-3
Encoding: UTF-8
LazyData: true
Collate:
 'generics.R'
 'struct_class.R'
 'parameter_class.R'
 'chart_class.R'
 'stato_class.R'
 'dataset_class.R'
 'entity_class.R'
 'entity_stato_class.R'
 'enum_class.R'
 'enum_stato_class.R'
 'output_class.R'
 'method_class.R'
 'model_class.R'
 'example_objects.R'
 'model_list_class.R'
 'metric_class.R'
 'iterator_class.R'
 'method_seq_class.R'
 'optimiser_class.R'
 'preprocess_class.R'
 'resampler_class.R'
 'struct.R'
 'struct_templates.R'
RoxygenNote: 6.1.1
Suggests: 
 testthat,
 rstudioapi,
 rmarkdown,
 covr,
 BiocStyle
VignetteBuilder: knitr
Imports: methods, ontologyIndex, crayon, datasets,graphics, stats, utils, knitr
biocViews: WorkflowStep

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.

grlloyd commented 5 years ago

Note that the struct package is a dependency for structToolbox (#1266)

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

25 November, 2019

I'm very sorry for letting this package slip out of the review cue.

For paired package submission, the way the new package build system works requires that the first package be submitted and built correctly, and then the second pacakge is added to the issue.

Please make the changes required for this pacakge to build successfully. I believe these are minor, including changing the version to 0.99.0. Once that is accomplished, please follow the instructions for submitting related packages

The below represent fairly substantial changes to your struct package; if you're willing to make these changes and continue with the review process then I will immediately provide comments on the structToolbox package.

struct package

Slots:

Name: name description data sample_meta variable_meta Class: character character data.frame data.frame data.frame

Name: type libraries Class: character character

Extends: "struct_class"

and `SummarizedExperiment`, e.g., for the `iris_dataset()` functionality one has

iris_SummarizedExperiment <- function () { iris = datasets::iris SummarizedExperiment::SummarizedExperiment( assay = list(t(iris[, 1:4])), colData = iris[, -(1:4), drop = FALSE] ) }

ise = iris_SummarizedExperiment() ise colData(ise) assay(ise) ise[, 1:10]

A significant limitation of this approach is the convention to represent 'features' as rows and samples as columns; it seems appropriate to think of a simple extenstion

.DatasetExperiment <- setClass( "DatasetExperiment", contains = "SummarizedExperiment"

Additional slots to match dataset

)

that allows the traditional columns-as-features representation

iris_DatasetExperiment <- function () { iris = datasets::iris .DatasetExperiment(SummarizedExperiment( assay = list(iris[, 1:4]), rowData = iris[, -(1:4), drop = FALSE] )) }

Note that the `assay()` container is agnostic to the type of data it contains, so long as it supports a 'matrix-like' API with `[,]` subsetting, `dim()`, and `dimnames()`, and that `SummarizedExperiment()` also includes a `metadata()` slot where perhaps some additional information can be stored.

- the use of `initialize()` methods (e.g., struct_class.R) has proven problematic, because of the complex contract the default method imposes -- a joint constructor and copy constructor. For this reason I suggest you create constructor methods `struct_class()` that call the 'internal' constructor, e.g.,

.struct_class <- setClass("struct_class", <...>) struct_class <- function(name, description, type, libraries, <...>) {

argument checking, etc

.struct_class(name = name, description = description, <...>)

}

The pattern for derived class constructors is

.derived <- setClass(contains = "struct_class", ...) derived <- function(derived_slot_1, <...>) { <...> .derived(struct_class(...), derived_slot_1 = derived_slot_1, ...) }

written so the derived class uses only the public API of the base class.

- if I understand correctly there are specific points where the user of your system might wish to extend it. Rather than relying on the user to figure out, as in your vignette, that they must

model_template=setClass('model_template', # replace model_template with ...

...your new model name

contains = c('model','stato'),       # stato is optional
does it make sense to provide a helper function

set_model_class <- function(name, slots, ..., where = .GlobalEnv) { setClass( name, contains = c('model', 'stato'), slots = slots, ..., where = where ) }

and similarly for methods on classes?

- it is confusing (e.g., dataset_class.R) to use `.` in a method name, since this is used by the S3 class system.

- all objects should have a `show` method that presents the user / developer with something that does not expose the internals of the data structure (slots, etc) but rather the API and a 'preview' of the data. These `show` methods should be defined to 'play well' with derived classes following some standardized scheme, e.g,. that the derived implementation always starts with `callNextMethod()`.

- some of the code formatting is suboptimal, e.g., in parameter_class.R
    if (name %in% valid)
    {return(TRUE)}
    else
    {return(FALSE)}
seems to be excessively compact (spacing beofre and after `{}`) and syntactically challenging for (human) parsing, e.g., copy-paste into an R session would fail. Generally the absence of spacing (e.g., around `=`) makes the code excessively difficult to parse

- personally I find a presentation like

cat( stato.id(obj), '\n', stato.name(obj), '\n', stato.definition(obj), '\n', '\nInputs:\n' )


simpler to parse and maintain than as it appears at stato_class.R:104.

- I'm not sure of the history of the redundant implementation, but most Bioconductor packages use `system.file(package="foo", "bar", "baz")` rather than `file.path(path.package("foo"), "bar", "baz")`.
bioc-issue-bot commented 4 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.

grlloyd commented 4 years ago

Thank you for your comments, they are very helpful. We are willing to follow your suggestions but as some of it requires quite significant changes it will take a bit of time, so please bear with us.

In the mean time we will make the more minor changes required to get the build checks to pass so that we can get feedback on structToolbox as well.

bioc-issue-bot commented 4 years ago

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

a0d90af change to version 0.99.0 for bioconductor 9b06c8f Delete struct.Rproj not meant to be tracked d40b3a0 update documentation... ...added transpose and xl... 919c3d4 add functions to create new struct classes... ...... f6adca6 update to include functions for adding struct clas... 62f86bf increase roxygen version to 7.0.1 d6d6ca5 fix broken test... ...due to updated show method ... 248caeb fix borken test... ...caused by update to show m... eb4ba1b Update .gitignore ignore rproj files 9d778d5 fix broken example 670e93a update docuementation 04229c3 Merge pull request #14 from computational-metabolo...

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

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

18b46d6 version bump to 0.99.1

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

grlloyd commented 4 years ago

AdditionalPackage: https://github.com/computational-metabolomics/structToolbox

bioc-issue-bot commented 4 years ago

Dear @grlloyd ,

You (or someone) has already posted that repository to our tracker.

See https://github.com/Bioconductor/Contributions/issues/1266

You cannot post the same repository more than once.

If you would like this repository to be linked to issue number: 1265, Please contact a Bioconductor Core Member.

mtmorgan commented 4 years ago

@lshep can you help clear this up, removing / closing issue 1266 ?

lshep commented 4 years ago

1266 has been close and the database updated. I'm kicking off a manual build of the package structToolbox now. If you have any further issues trying to get builds for the package please ping me. Also make sure your webhook is set up on the structToolbox package to be able to get build reports on version bumps.

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

mtmorgan commented 4 years ago

This package depends on pmp, but pmp is still under review. It seems the review of the continued review of the current package must wait until pmp has been accepted.

grlloyd commented 4 years ago

yes, the same will be be true for the sbcms package as well. We will need to make some changes to structToolbox that are related to the changes in response to comments made for struct, so we can work on these changes while we wait for pmp and sbcms.

bioc-issue-bot commented 4 years ago

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

29581bb tidy code formatting add comments, put /n on new ... cd73d59 change file.path to sytem.file preferred by bioco... 4798de5 add whitespace to make more readable specifically... e9a1270 make formatting of curly brackets consistent de14bbb add/update show methods for objects 11286ba add/update show methods for objects aa56d32 change method with dot to underscore fe497b7 rename method with dot .... ... to use underscore... ef36196 use class constructors... ...and stop using 'init... a05cf11 add DatasetExperiment object... ...replaces datas... 6216190 increase version to 0.99.2 14db8f4 Merge pull request #15 from computational-metabolo...

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

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

1e26b3b Update DESCRIPTION

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

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

f032b2b version bump 0.99.3 to trigger bioconductor build

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

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

df4cae4 version bump for bioconductor build

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

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

8db793a use class contructors and... ...rename all functi... 56a019f fix broken tests and... ...update some documentat... c187e3e cmd check fixes (see description) changed @param ... 7af536f add @return to documentation 5c731ac add Remotes.... ...temporarily until dependencies... 6db9c64 add Remotes... ...temporarily until dependencies ... 50414c0 add as_dataframe method 00e5eca use DatasetExperiment indexing to remove features 1061573 use PCA from structToolbox instead of built-in e5f7846 change to use internal pls functions for orthogona... c58b2dc minor text fixes 97c0c24 remove remotes for pmp and sbcms sbcms merged int... f388a80 update to pmp 0.99.2 89a3c5d update to pmp 0.99.2 a316092 add glog opt plot class 8329945 change "list" to "allowed" for enums b3b2ba0 remove all params and outputs_ tags also fix res... 9bf1542 incremental changes to use struct class constructo... a110b7e update to use new struct class constructors acec9d5 fix/update tests 4391cdb fix/update examples 34c7daf Merge branch 'master' into change_to_struct_v0.99.... 983ec2e Merge pull request #5 from computational-metabolom...

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

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

b1e0ed6 Delete structtoolbox.Rproj b703c81 set depends R version >=4.0 e3c5f14 version bump to trigger bioconductor build

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

Is this package ready for review, now that pmp has been accepted?

grlloyd commented 4 years ago

I would like to make a few minor changes to structToolbox first. I will probably do this tomorrow. I will also provide a detailed response to your previous comments re struct, which required making quite a few changes to the toolbox as well.

bioc-issue-bot commented 4 years ago

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

40be1b5 rename to use underscores instead of dot 37c5b1e rename sbcms to MTBLS79 a733fba update tests/vignettes after renaming

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

grlloyd commented 4 years ago

Dear Martin, thank you for comprehensive and helpful review. Please see my response below, where I have tried to address your comments on struct.

I'm very concerned about the lack of interoperability with standard Bioconductor data representations...

I have replaced the dataset class with DatasetExperiment, as suggested. I have added as.SummarizedExperiment to convert back from DE to SE. I have also simplified the class structure so that multiple inheritance is avoided for input/output slots.

The use of initialize() methods (e.g., struct_class.R) has proven problematic… For this reason I suggest you create constructor methods.

I have followed your advice and converted all classes to use constructors instead of initialize(). I have also applied this change to structToolbox. Corresponding documentation, examples, tests etc have also been updated.

if I understand correctly there are specific points where the user of your system might wish to extend it. Rather than relying on the user to figure out…does it make sense to provide a helper function?

I much prefer this and I have replaced the original approach with the following helper functions: set_struct_obj, set_obj_method and set_obj_show, and updated the vignette.

it is confusing (e.g., dataset_class.R) to use . in a method name, since this is used by the S3 class system.

I have renamed all class objects and slots that used a . with names that use an underscore. The only remaining functions are as. methods (might be better as S3?) , and two new slots .params and .outputs where the . indicates a "private" slot.

all objects should have a show method…

I have updated the show functions and use callnextmethod() so that derived methods can build on the output of the class they inherit.

some of the code formatting is suboptimal

I have tried to improve this throughout the package.

personally I find a presentation like…simpler to parse and maintain than as it appears at stato_class.R:104

I have tried to improve this throughout the package.

I'm not sure of the history of the redundant implementation, but most Bioconductor packages use system.file(package="foo", "bar", "baz")

I have replaced package.path with thesystem.fileapproach in all cases.

In addition, I have updated structToolbox to work with the latest release of pmp, and it is now ready for review.

bioc-issue-bot commented 4 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 4 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/grlloyd.keys is not empty), then no further steps are required. Otherwise, do the following:

  1. Add an SSH key to your github account
  2. Submit your SSH key to Bioconductor

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("struct"). The package 'landing page' will be created at

https://bioconductor.org/packages/struct

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.

mtmorgan commented 4 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/grlloyd.keys is not empty), then no further steps are required. Otherwise, do the following:

  1. Add an SSH key to your github account
  2. Submit your SSH key to Bioconductor

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("structToolbox"). The package 'landing page' will be created at

https://bioconductor.org/packages/structToolbox

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.