New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InterMineR #393

Closed
kostaskyritsis opened this Issue Jun 8, 2017 · 48 comments

Comments

Projects
None yet
5 participants
@kostaskyritsis

kostaskyritsis commented Jun 8, 2017

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 understand that by submitting my package to Bioconductor,
    the package source and all review commentary are visible to the
    general public.

  • I have read the Bioconductor Package Submission
    instructions. My package is consistent with the Bioconductor
    Package Guidelines.

  • 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.

  • My package addresses statistical or bioinformatic issues related
    to the analysis and comprehension of high throughput genomic data.

  • 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:

  • The 'devel' branch for new packages and features.
  • The stable 'release' branch, made available every six
    months, for bug fixes.
  • 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

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Jun 8, 2017

Collaborator

Hi @kostaskyritsis

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: InterMineR
Title: R Interface with InterMine-Powered Databases
Version: 0.99.0
Date: 2016-01-05
Author: Bing Wang, Julie Sullivan, Rachel Lyne, Konstantinos Kyritsis
Maintainer: InterMine Team <j.sullivan@gen.cam.ac.uk>
Description: Databases based on the InterMine platform such as FlyMine, modMine (modENCODE), RatMine, YeastMine, HumanMine and TargetMine are integrated databases of genomic, expression and protein data for various organisms. Integrating data makes it possible to run sophisticated data mining queries that span domains of biological knowledge. This R package provides interfaces with these databases through webservices. It makes most from the correspondence of the data frame object in R and the table object in databases, while hiding the details of data exchange through XML or JSON.
License: LGPL
VignetteBuilder: knitr
Imports: Biostrings, RCurl, XML, xml2, RJSONIO, sqldf, igraph, httr
Suggests: BiocStyle, Gviz, knitr, rmarkdown
BugReports: https://github.com/intermine/intermineR/issues
biocViews: GeneExpression, SNP, GeneSetEnrichment, DifferentialExpression, GeneRegulation, GenomeAnnotation, GenomeWideAssociation, FunctionalPrediction, AlternativeSplicing, ComparativeGenomics, FunctionalGenomics, Proteomics, SystemsBiology, Microarray, MultipleComparison, Pathways, GO, KEGG, Reactome, Visualization

Collaborator

bioc-issue-bot commented Jun 8, 2017

Hi @kostaskyritsis

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: InterMineR
Title: R Interface with InterMine-Powered Databases
Version: 0.99.0
Date: 2016-01-05
Author: Bing Wang, Julie Sullivan, Rachel Lyne, Konstantinos Kyritsis
Maintainer: InterMine Team <j.sullivan@gen.cam.ac.uk>
Description: Databases based on the InterMine platform such as FlyMine, modMine (modENCODE), RatMine, YeastMine, HumanMine and TargetMine are integrated databases of genomic, expression and protein data for various organisms. Integrating data makes it possible to run sophisticated data mining queries that span domains of biological knowledge. This R package provides interfaces with these databases through webservices. It makes most from the correspondence of the data frame object in R and the table object in databases, while hiding the details of data exchange through XML or JSON.
License: LGPL
VignetteBuilder: knitr
Imports: Biostrings, RCurl, XML, xml2, RJSONIO, sqldf, igraph, httr
Suggests: BiocStyle, Gviz, knitr, rmarkdown
BugReports: https://github.com/intermine/intermineR/issues
biocViews: GeneExpression, SNP, GeneSetEnrichment, DifferentialExpression, GeneRegulation, GenomeAnnotation, GenomeWideAssociation, FunctionalPrediction, AlternativeSplicing, ComparativeGenomics, FunctionalGenomics, Proteomics, SystemsBiology, Microarray, MultipleComparison, Pathways, GO, KEGG, Reactome, Visualization

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Jun 8, 2017

Collaborator

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.

Collaborator

bioc-issue-bot commented Jun 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Jun 8, 2017

Collaborator

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

Collaborator

bioc-issue-bot commented Jun 8, 2017

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

@lawremi

This comment has been minimized.

Show comment
Hide comment
@lawremi

lawremi Jun 8, 2017

Member

This needs a layer that integrates it with Bioconductor, e.g., returns annotations as GRanges, expression data as SummarizedExperiments, etc. It also needs a more convenient query interface. The template selection seems to expose an implementation detail. Why not have a function for each type of query? Why not have constraints expressed as R expressions that are translated? Why not have formal classes representing the query and data model?

Member

lawremi commented Jun 8, 2017

This needs a layer that integrates it with Bioconductor, e.g., returns annotations as GRanges, expression data as SummarizedExperiments, etc. It also needs a more convenient query interface. The template selection seems to expose an implementation detail. Why not have a function for each type of query? Why not have constraints expressed as R expressions that are translated? Why not have formal classes representing the query and data model?

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Jun 13, 2017

Collaborator

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

2255206 Create InterMineR.Rmd
d454f0d Create DESCRIPTION
ea20de5 Merge pull request #17 from intermine/dev Dev

Collaborator

bioc-issue-bot commented Jun 13, 2017

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

2255206 Create InterMineR.Rmd
d454f0d Create DESCRIPTION
ea20de5 Merge pull request #17 from intermine/dev Dev

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Jun 13, 2017

Collaborator

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

Collaborator

bioc-issue-bot commented Jun 13, 2017

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

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Jun 14, 2017

Collaborator

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

ed759d8 Update DESCRIPTION
6c8375e Merge pull request #20 from intermine/dev Update ...

Collaborator

bioc-issue-bot commented Jun 14, 2017

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

ed759d8 Update DESCRIPTION
6c8375e Merge pull request #20 from intermine/dev Update ...

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Jun 14, 2017

Collaborator

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

Collaborator

bioc-issue-bot commented Jun 14, 2017

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

@LiNk-NY

This comment has been minimized.

Show comment
Hide comment
@LiNk-NY

LiNk-NY Jun 26, 2017

Hi Konstantinos @kostaskyritsis,
I'm trying to build the package locally but I am getting an error:

* checking examples ... ERROR
Running examples in ‘InterMineR-Ex.R’ failed
The error most likely occurred in:
[truncated]
Error in rsqlite_send_query(conn@ptr, statement) : no such table: res
Calls: getModel ... initialize -> initialize -> rsqlite_send_query -> .Call
Execution halted

Regards,
Marcel

LiNk-NY commented Jun 26, 2017

Hi Konstantinos @kostaskyritsis,
I'm trying to build the package locally but I am getting an error:

* checking examples ... ERROR
Running examples in ‘InterMineR-Ex.R’ failed
The error most likely occurred in:
[truncated]
Error in rsqlite_send_query(conn@ptr, statement) : no such table: res
Calls: getModel ... initialize -> initialize -> rsqlite_send_query -> .Call
Execution halted

Regards,
Marcel

@kostaskyritsis

This comment has been minimized.

Show comment
Hide comment
@kostaskyritsis

kostaskyritsis Jun 27, 2017

Hi Marcel @LiNk-NY ,

I have cloned locally the current intermine/InterMineR master branch and performed R CMD build and R CMD check in both Windows:

R version 3.4.0 Patched (2017-05-25 r72747)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows >= 8 x64 (build 9200)

and Ubuntu OS:

R version 3.4.0 (2017-04-21)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 16.04.2 LTS

but I could not reproduce the error that you mention.

Similarly to this travis ci check:

https://travis-ci.org/kostaskyritsis/InterMineR/builds/247484749?utm_source=github_status&utm_medium=notification

The error appears to originate from the example code within an .Rd file.

Would it be possible to send me the specific code included within the
[truncated]
part of the error message that you mention above? This way I will locate the Rd file containing the problem.

Furthermore, concerning the comment of @lawremi , we have already created and now testing two functions that facilitate conversion of InterMineR query results to GRanges and RangedSummarizedExperiment objects.

Also we are are working on a formal class to represent queries and on functions that will provide a more convenient query interface than the current system of defining list objects.

We will update shortly with a version bump when these changes are ready.

Best,
Konstantinos

kostaskyritsis commented Jun 27, 2017

Hi Marcel @LiNk-NY ,

I have cloned locally the current intermine/InterMineR master branch and performed R CMD build and R CMD check in both Windows:

R version 3.4.0 Patched (2017-05-25 r72747)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows >= 8 x64 (build 9200)

and Ubuntu OS:

R version 3.4.0 (2017-04-21)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 16.04.2 LTS

but I could not reproduce the error that you mention.

Similarly to this travis ci check:

https://travis-ci.org/kostaskyritsis/InterMineR/builds/247484749?utm_source=github_status&utm_medium=notification

The error appears to originate from the example code within an .Rd file.

Would it be possible to send me the specific code included within the
[truncated]
part of the error message that you mention above? This way I will locate the Rd file containing the problem.

Furthermore, concerning the comment of @lawremi , we have already created and now testing two functions that facilitate conversion of InterMineR query results to GRanges and RangedSummarizedExperiment objects.

Also we are are working on a formal class to represent queries and on functions that will provide a more convenient query interface than the current system of defining list objects.

We will update shortly with a version bump when these changes are ready.

Best,
Konstantinos

@LiNk-NY

This comment has been minimized.

Show comment
Hide comment
@LiNk-NY

LiNk-NY Jun 27, 2017

Hi Konstantinos, @kostaskyritsis

Here it is:

* checking examples ... ERROR
Running examples in ‘InterMineR-Ex.R’ failed
The error most likely occurred in:
> ### Name: getModel
> ### Title: Get the model of InterMine
> ### Aliases: getModel
> 
> ### ** Examples
> 
> im <- initInterMine("humanmine.org/humanmine")
> 
> model <- getModel(im)
Loading required package: tcltk
Error in rsqlite_send_query(conn@ptr, statement) : no such table: res
Calls: getModel ... initialize -> initialize -> rsqlite_send_query -> .Call
Execution halted

R version 3.4.0 Patched (2017-04-28 r72639)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 16.04.2 LTS

Regards,
Marcel

LiNk-NY commented Jun 27, 2017

Hi Konstantinos, @kostaskyritsis

Here it is:

* checking examples ... ERROR
Running examples in ‘InterMineR-Ex.R’ failed
The error most likely occurred in:
> ### Name: getModel
> ### Title: Get the model of InterMine
> ### Aliases: getModel
> 
> ### ** Examples
> 
> im <- initInterMine("humanmine.org/humanmine")
> 
> model <- getModel(im)
Loading required package: tcltk
Error in rsqlite_send_query(conn@ptr, statement) : no such table: res
Calls: getModel ... initialize -> initialize -> rsqlite_send_query -> .Call
Execution halted

R version 3.4.0 Patched (2017-04-28 r72639)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 16.04.2 LTS

Regards,
Marcel

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Jul 3, 2017

Collaborator

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

d907577 Update DESCRIPTION

Collaborator

bioc-issue-bot commented Jul 3, 2017

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

d907577 Update DESCRIPTION

@LiNk-NY

This comment has been minimized.

Show comment
Hide comment
@LiNk-NY

LiNk-NY Aug 10, 2017

Hi Konstantinos, @kostaskyritsis
Thanks for making those changes. I will review them shortly.
Regards,
Marcel

LiNk-NY commented Aug 10, 2017

Hi Konstantinos, @kostaskyritsis
Thanks for making those changes. I will review them shortly.
Regards,
Marcel

@LiNk-NY

This comment has been minimized.

Show comment
Hide comment
@LiNk-NY

LiNk-NY Aug 14, 2017

Hi Konstantinos, @kostaskyritsis
Thank you for submitting to Bioconductor.
Please find the review below.
Thank you.

Regards,
Marcel


InterMineR #393

DESCRIPTION

  • Provide proper format for the DESCRIPTION file. It becomes hard to read
    when greater than 80 column width.
  • Please add a Depends field to your DESCRIPTION

NAMESPACE

  • Consider using roxygen2 to list imports and exports

LICENSE

  • Please rename your LICENCE file to LICENSE.

R

  • Use requireNamespace to test for available suggested package
  • Avoid the use of sapply, use the more robust vapply
  • Use default arguments in function definitions
  • Avoid the use of assign. Create a list with available arguments and use
    do.call to
    call the function if it must be done this way.
  • Use shorter error messages or collapse to fit at most 2 lines
  • Avoid using unnecessary logic, e.g. checking for values in strand.vector
    before gsubbing
  • Avoid growing a vector c(), use the allocate and fill strategy
  • Use extractor functions for accessing data elements of the class, avoid using
    @ and use where() instead
  • There's no need to convert logical vectors to numeric indices, just use
    the logical vector for subsetting
  • Why not create a list where you filter out NULL and combine it to
    inheritQuery if available? (see setQuery.R)

Minor:

  • Test S3 classes with is.* such as is.character
  • Code should be well formatted and not exceed 80 column width
  • Avoid repetitive code, write and reuse functions for such repetitive tasks
  • Whenever possible avoid using nested lists to store data
  • Use the more robust [[ to access elements in a list rather than $

vignettes

  • Vignette looks good

LiNk-NY commented Aug 14, 2017

Hi Konstantinos, @kostaskyritsis
Thank you for submitting to Bioconductor.
Please find the review below.
Thank you.

Regards,
Marcel


InterMineR #393

DESCRIPTION

  • Provide proper format for the DESCRIPTION file. It becomes hard to read
    when greater than 80 column width.
  • Please add a Depends field to your DESCRIPTION

NAMESPACE

  • Consider using roxygen2 to list imports and exports

LICENSE

  • Please rename your LICENCE file to LICENSE.

R

  • Use requireNamespace to test for available suggested package
  • Avoid the use of sapply, use the more robust vapply
  • Use default arguments in function definitions
  • Avoid the use of assign. Create a list with available arguments and use
    do.call to
    call the function if it must be done this way.
  • Use shorter error messages or collapse to fit at most 2 lines
  • Avoid using unnecessary logic, e.g. checking for values in strand.vector
    before gsubbing
  • Avoid growing a vector c(), use the allocate and fill strategy
  • Use extractor functions for accessing data elements of the class, avoid using
    @ and use where() instead
  • There's no need to convert logical vectors to numeric indices, just use
    the logical vector for subsetting
  • Why not create a list where you filter out NULL and combine it to
    inheritQuery if available? (see setQuery.R)

Minor:

  • Test S3 classes with is.* such as is.character
  • Code should be well formatted and not exceed 80 column width
  • Avoid repetitive code, write and reuse functions for such repetitive tasks
  • Whenever possible avoid using nested lists to store data
  • Use the more robust [[ to access elements in a list rather than $

vignettes

  • Vignette looks good
@LiNk-NY

This comment has been minimized.

Show comment
Hide comment
@LiNk-NY

LiNk-NY Aug 22, 2017

Any updates on this Konstantinos? @kostaskyritsis

LiNk-NY commented Aug 22, 2017

Any updates on this Konstantinos? @kostaskyritsis

@kostaskyritsis

This comment has been minimized.

Show comment
Hide comment
@kostaskyritsis

kostaskyritsis Aug 22, 2017

Hi Marcel, @LiNk-NY
I have been working on adding some tutorials and updating the vignette in order to facilitate the usage of the package.

Presently I will perform:

  1. An update on the README.md file and a version bump, and
  2. A second update and version bump containing the changes that you suggested followed by a comment about them as well.

I will make these changes and comment on them very soon!
All best,
Konstantinos

kostaskyritsis commented Aug 22, 2017

Hi Marcel, @LiNk-NY
I have been working on adding some tutorials and updating the vignette in order to facilitate the usage of the package.

Presently I will perform:

  1. An update on the README.md file and a version bump, and
  2. A second update and version bump containing the changes that you suggested followed by a comment about them as well.

I will make these changes and comment on them very soon!
All best,
Konstantinos

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Aug 23, 2017

Collaborator

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

45dc983 Update DESCRIPTION

Collaborator

bioc-issue-bot commented Aug 23, 2017

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

45dc983 Update DESCRIPTION

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Aug 23, 2017

Collaborator

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

Collaborator

bioc-issue-bot commented Aug 23, 2017

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

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Aug 28, 2017

Collaborator

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

7dc1a6b Update DESCRIPTION

Collaborator

bioc-issue-bot commented Aug 28, 2017

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

7dc1a6b Update DESCRIPTION

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Aug 28, 2017

Collaborator

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

Collaborator

bioc-issue-bot commented Aug 28, 2017

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

@kostaskyritsis

This comment has been minimized.

Show comment
Hide comment
@kostaskyritsis

kostaskyritsis Aug 28, 2017

Hi Marcel, @LiNk-NY

Based on your review I performed the following changes:

DESCRIPTION:

  • Used GitHub's Soft Wrap and manually edited where necessary.
  • Added Depends field.

NAMESPACE:

  • Used roxygen2 format:

#' export
#' import
#' importFrom

and RStudio to create NAMESPACE file for the package

LICENSE:

  • Corrected

R:

  • Replaced isNamespaceLoaded with requireNamespace to test for available suggested package GeneAnswers
  • Replaced sapply with vapply
  • The package works with many different InterMine instances and the data requested by a query can vary greatly but I tried to add default values on function arguments wherever possible.
  • I avoided the usage of assign and instead I used the proposed do.call to pass a list containing the arguments and call the function (in convertToGeneAnswers.R)
  • Shortened error messages to fit at most two lines
  • Removed unnecessary logic (e.g. strand.vector)
  • Replaced @ in accessing elements of a class with the function slot
  • Avoided unnecessary convertion of logical to numerical indexes
    (an exception exists in convertToRangedSummarizedExperiment function where multiple indices are combined in a single vector)
  • inheritQuery is used to pass the values of a pre-existing list query (e.g. from the variety of choices from the template queries) to the new InterMineR-class query.

Perhaps it would be better to avoid combining the rest of the arguments with those of inheritQuery because:

  1. The goal is for the user to be able to also modify the query within the setQuery function
  2. To be able to modify the query in a straightforward manner (if any other argument is assigned with a value in setQuery then it replaces the value from the inheritQuery (also mentioned in the documentation details setQuery.Rd). This way it is easier to replace, add or remove a constraint, a value from the select argument etc.

Minor:

  • I only defined the tested the S4 class InterMineR and its elements can not be of different class than the one defined within the representation
  • The code has been edited and does not exceeds 80 column width
  • Replaced $ with [[ to access elements in a list

Furthermore I added the RMD files:

  1. Enrichment_Analysis_and_Visualization.Rmd
  2. FlyMine_Genomic_Visualizations.Rmd

as tutorials facilitating the usage of the package and updated the vignette:

  • InterMineR.Rmd

Please let me know if more changes are necessary for the acceptance of the InterMineR package.

All best,
Konstantinos

kostaskyritsis commented Aug 28, 2017

Hi Marcel, @LiNk-NY

Based on your review I performed the following changes:

DESCRIPTION:

  • Used GitHub's Soft Wrap and manually edited where necessary.
  • Added Depends field.

NAMESPACE:

  • Used roxygen2 format:

#' export
#' import
#' importFrom

and RStudio to create NAMESPACE file for the package

LICENSE:

  • Corrected

R:

  • Replaced isNamespaceLoaded with requireNamespace to test for available suggested package GeneAnswers
  • Replaced sapply with vapply
  • The package works with many different InterMine instances and the data requested by a query can vary greatly but I tried to add default values on function arguments wherever possible.
  • I avoided the usage of assign and instead I used the proposed do.call to pass a list containing the arguments and call the function (in convertToGeneAnswers.R)
  • Shortened error messages to fit at most two lines
  • Removed unnecessary logic (e.g. strand.vector)
  • Replaced @ in accessing elements of a class with the function slot
  • Avoided unnecessary convertion of logical to numerical indexes
    (an exception exists in convertToRangedSummarizedExperiment function where multiple indices are combined in a single vector)
  • inheritQuery is used to pass the values of a pre-existing list query (e.g. from the variety of choices from the template queries) to the new InterMineR-class query.

Perhaps it would be better to avoid combining the rest of the arguments with those of inheritQuery because:

  1. The goal is for the user to be able to also modify the query within the setQuery function
  2. To be able to modify the query in a straightforward manner (if any other argument is assigned with a value in setQuery then it replaces the value from the inheritQuery (also mentioned in the documentation details setQuery.Rd). This way it is easier to replace, add or remove a constraint, a value from the select argument etc.

Minor:

  • I only defined the tested the S4 class InterMineR and its elements can not be of different class than the one defined within the representation
  • The code has been edited and does not exceeds 80 column width
  • Replaced $ with [[ to access elements in a list

Furthermore I added the RMD files:

  1. Enrichment_Analysis_and_Visualization.Rmd
  2. FlyMine_Genomic_Visualizations.Rmd

as tutorials facilitating the usage of the package and updated the vignette:

  • InterMineR.Rmd

Please let me know if more changes are necessary for the acceptance of the InterMineR package.

All best,
Konstantinos

@LiNk-NY

This comment has been minimized.

Show comment
Hide comment
@LiNk-NY

LiNk-NY Aug 28, 2017

Hi Konstantinos, @kostaskyritsis

Thank you for making those changes. I will have a quick look at them and will get back to you shortly.

Note: The point about not using @ is to use a extractor function that describes what is being extracted as in assay(SummarizedExperiment) instead of SummarizedExperiment@assay.

Regards,
Marcel

LiNk-NY commented Aug 28, 2017

Hi Konstantinos, @kostaskyritsis

Thank you for making those changes. I will have a quick look at them and will get back to you shortly.

Note: The point about not using @ is to use a extractor function that describes what is being extracted as in assay(SummarizedExperiment) instead of SummarizedExperiment@assay.

Regards,
Marcel

@LiNk-NY

This comment has been minimized.

Show comment
Hide comment
@LiNk-NY

LiNk-NY Sep 6, 2017

Hi Konstantinos, @kostaskyritsis
The use of slot is acceptable only if the developer will be accessing that slot. Otherwise, I would recommend that you provide an intelligible function name for access to the slot, usually of the same name (see example above).
Regards,
Marcel

LiNk-NY commented Sep 6, 2017

Hi Konstantinos, @kostaskyritsis
The use of slot is acceptable only if the developer will be accessing that slot. Otherwise, I would recommend that you provide an intelligible function name for access to the slot, usually of the same name (see example above).
Regards,
Marcel

@LiNk-NY

This comment has been minimized.

Show comment
Hide comment
@LiNk-NY

LiNk-NY Sep 6, 2017

I would also break up error messages using commas ( , ) so that there is no overflow across lines.

Example:
https://github.com/intermine/InterMineR/blob/master/R/convertToGeneAnswers.R#L58

stop("'enrichIdentifier' from getWidgets function is missing,",
"\n Assign 'enrichCategoryChildName' with the appropriate identifier.")

LiNk-NY commented Sep 6, 2017

I would also break up error messages using commas ( , ) so that there is no overflow across lines.

Example:
https://github.com/intermine/InterMineR/blob/master/R/convertToGeneAnswers.R#L58

stop("'enrichIdentifier' from getWidgets function is missing,",
"\n Assign 'enrichCategoryChildName' with the appropriate identifier.")
@LiNk-NY

This comment has been minimized.

Show comment
Hide comment
@LiNk-NY

LiNk-NY Sep 6, 2017

Also, for the minor point:

  • Test S3 classes with is.* such as is.character

This means that you should test class membership using the special function rather than comparing class strings. For example:

class(where) != "list"
      # VS #
!is.list(where)

The latter method is more robust.

Regards,
Marcel

LiNk-NY commented Sep 6, 2017

Also, for the minor point:

  • Test S3 classes with is.* such as is.character

This means that you should test class membership using the special function rather than comparing class strings. For example:

class(where) != "list"
      # VS #
!is.list(where)

The latter method is more robust.

Regards,
Marcel

@kostaskyritsis

This comment has been minimized.

Show comment
Hide comment
@kostaskyritsis

kostaskyritsis Sep 7, 2017

Hi Marcel, @LiNk-NY

Thank you, I will make these changes and comment on them very soon!

All best,
Konstantinos

kostaskyritsis commented Sep 7, 2017

Hi Marcel, @LiNk-NY

Thank you, I will make these changes and comment on them very soon!

All best,
Konstantinos

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Sep 10, 2017

Collaborator

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

362d825 Update DESCRIPTION

Collaborator

bioc-issue-bot commented Sep 10, 2017

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

362d825 Update DESCRIPTION

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Sep 10, 2017

Collaborator

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

Collaborator

bioc-issue-bot commented Sep 10, 2017

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

@kostaskyritsis

This comment has been minimized.

Show comment
Hide comment
@kostaskyritsis

kostaskyritsis Sep 10, 2017

Hi Marcel, @LiNk-NY

Regarding the latest changes in the InterMineR package:

  • I created methods specifically for accessing the objects of InterMineR-class, which can be found in the InterMineR-methods.R (for the code) and InterMineR-methods.Rd (for the documentation), and removed the function slot from the code which is accessed by the user.
  • I added commas and the newline special character '\n' where it was required for the stop function (convertToGRanges.R, convertToGeneAnswers.R, setConstraints.R).
  • I replaced any logical comparison using the class function with the appropriate is.* (convertToGRanges.R, setConstraints.R, setQuery.R, simplifyResult.R)

All best,
Konstantinos

kostaskyritsis commented Sep 10, 2017

Hi Marcel, @LiNk-NY

Regarding the latest changes in the InterMineR package:

  • I created methods specifically for accessing the objects of InterMineR-class, which can be found in the InterMineR-methods.R (for the code) and InterMineR-methods.Rd (for the documentation), and removed the function slot from the code which is accessed by the user.
  • I added commas and the newline special character '\n' where it was required for the stop function (convertToGRanges.R, convertToGeneAnswers.R, setConstraints.R).
  • I replaced any logical comparison using the class function with the appropriate is.* (convertToGRanges.R, setConstraints.R, setQuery.R, simplifyResult.R)

All best,
Konstantinos

@LiNk-NY

This comment has been minimized.

Show comment
Hide comment
@LiNk-NY

LiNk-NY Sep 11, 2017

Hi Konstantinos, @kostaskyritsis
Thank you for making those changes. Your package has been accepted.
Thanks for submitting to Bioconductor.
Best regards,
Marcel

LiNk-NY commented Sep 11, 2017

Hi Konstantinos, @kostaskyritsis
Thank you for making those changes. Your package has been accepted.
Thanks for submitting to Bioconductor.
Best regards,
Marcel

@bioc-issue-bot

This comment has been minimized.

Show comment
Hide comment
@bioc-issue-bot

bioc-issue-bot Sep 11, 2017

Collaborator

Your package has been accepted. It will be added to the
Bioconductor Git repository and nightly builds. Additional
information will be sent to the maintainer email address in
the next several days.

Thank you for contributing to Bioconductor!

Collaborator

bioc-issue-bot commented Sep 11, 2017

Your package has been accepted. It will be added to the
Bioconductor Git repository and nightly builds. Additional
information will be sent to the maintainer email address in
the next several days.

Thank you for contributing to Bioconductor!

@kostaskyritsis

This comment has been minimized.

Show comment
Hide comment
@kostaskyritsis

kostaskyritsis Sep 12, 2017

Great! Thanks Marcel, @LiNk-NY!

kostaskyritsis commented Sep 12, 2017

Great! Thanks Marcel, @LiNk-NY!

@mtmorgan

This comment has been minimized.

Show comment
Hide comment
@mtmorgan

mtmorgan Sep 27, 2017

Contributor

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 (tithub.com/.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 biocLite("YOUR_PACKAGE_NAME"). The package 'landing page' will be created at

https://bioconductor.org/packages/YOUR_PACKAGE_NAME

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.

Contributor

mtmorgan commented Sep 27, 2017

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 (tithub.com/.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 biocLite("YOUR_PACKAGE_NAME"). The package 'landing page' will be created at

https://bioconductor.org/packages/YOUR_PACKAGE_NAME

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 mtmorgan closed this Sep 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment