kostaskyritsis 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/intermine/InterMineR/tree/master

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

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

lawremi commented 7 years ago

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

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

bioc-issue-bot commented 7 years ago

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

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

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

LiNk-NY commented 7 years ago

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

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

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

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

d907577 Update DESCRIPTION

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

LiNk-NY commented 7 years ago

Hi Konstantinos, @kostaskyritsis cc: @julie-sullivan

After updating my installed packages, I am no longer getting the error mentioned earlier. I will review your package soon.

Also, please keep in mind that if you are planning to submit to CRAN, your package will not be eligible for Bioconductor. We don't allow duplicate packages.

Thank you, Marcel

bioc-issue-bot commented 7 years ago

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

9e62213 Update DESCRIPTION

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: "ABNORMAL". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

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

bioc-issue-bot commented 7 years ago

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

da6b48f Update DESCRIPTION

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

kostaskyritsis commented 7 years ago

Hi Marcel @LiNk-NY cc: @lawremi

Thank you for your comment, yes we plan to submit the InterMineR package only in Bioconductor.

Concerning your review, we have added two formal classes:

InterMineR_query
InterMineR_result

along with 4 new functions:

setConstraints
setQuery
runQuery2
InterMineR_Query2XML

in order to facilitate the query process with InterMineR package and avoid defining multiple list objects and loops. Furthermore we have added the functions:

convertToGRanges
convertToRangedSummarizedExperiment

in order to facilitate the conversion of genomic data and annotations retrieved by InterMineR to GRanges and RangedSummarizedExperiment objects.

These changes were included in the latest push from the intermine/InterMineR master branch.

All best, Konstantinos

bioc-issue-bot commented 7 years ago

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

9a092a9 Update DESCRIPTION

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

LiNk-NY commented 7 years ago

Hi Konstantinos, @kostaskyritsis It would be advisable to keep your naming conventions consistent. Please use camelCase naming conventions for your functions. Appending "query" and "result" to a class name is not really helpful for the user. Please use class names and methods that are more generic. Perhaps, you can create an "InterMineR" class with query and result methods? I will look into your package shortly. Regards, Marcel

kostaskyritsis commented 7 years ago

Hi Marcel, @LiNk-NY Thank you for your comment! I will replace InterMineR_query and InterMineR_result classes with a class called InterMineR and add the appropriate methods. Also I will check the rest of the functions and modify them accordingly to follow the camelCase naming conventions. All best, Konstantinos

LiNk-NY commented 7 years ago

Hi Konstantinos, @kostaskyritsis Any updates on making these changes? Please don't forget to bump your package's version when you do.

Regards, Marcel

bioc-issue-bot commented 7 years ago

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

421e8a7 Update DESCRIPTION

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

kostaskyritsis commented 7 years ago

Hi Marcel, @LiNk-NY

I have updated the package and it was built without errors or warnings!

Functions, methods and classes follow the camelCase naming conventions now. I removed InterMineR_query and InterMineR_result classes and created a single InterMineR formal class for complex queries, with runQuery and summary as generic functions that call methods for this class.

Also the package has now two datasets, which can be loaded with the data() function, to facilitate creating examples in documentation.

Finally, I added one more function, convertToGeneAnswers, to facilitate visualization of InterMineR enrichment analysis results with the functions of GeneAnswers Bioconductor package.

All best, Konstantinos

LiNk-NY commented 7 years ago

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

LiNk-NY commented 7 years ago

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

Any updates on this Konstantinos? @kostaskyritsis

kostaskyritsis commented 7 years ago

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:

An update on the README.md file and a version bump, and
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 commented 7 years ago

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

45dc983 Update DESCRIPTION

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

bioc-issue-bot commented 7 years ago

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

7dc1a6b Update DESCRIPTION

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

kostaskyritsis commented 7 years ago

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

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

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

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

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

Hi Marcel, @LiNk-NY

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

All best, Konstantinos

bioc-issue-bot commented 7 years ago

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

362d825 Update DESCRIPTION

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

kostaskyritsis commented 7 years ago

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

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

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

Great! Thanks Marcel, @LiNk-NY!

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

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.

Bioconductor / Contributions

InterMineR #393

InterMineR #393

DESCRIPTION

NAMESPACE

LICENSE

R

vignettes

' export

' import

' importFrom