(inactive) gwasrapidd

[ramiromagno]

Note

Please note that I am aware that BiocCheck::BiocCheck() should give no errors and no warnings. I have one warning still:

"The following files are over 5MB in size: '.git/objects/pack/pack-fb9aec1c48d78e3166698db47359a703612078be.pack'"

I have followed the instructions here: https://bioconductor.org/developers/how-to/git/remove-large-data/ but to no avail.

Submission form

Update the following URL to point to the GitHub repository of
the package you wish to submit to Bioconductor

• Repository: https://github.com/ramiromagno/gwasrapidd

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

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

[bioc-issue-bot]

Hi @ramiromagno

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: gwasrapidd
Type: Package
Title: REST API Client for the NHGRI-EBI GWAS Catalog
Version: 0.0.1
Authors@R: c(
    person(given = "Ramiro", family = "Magno", email = "ramiro.magno@gmail.com",
    role = c("aut", "cre"), comment = c(ORCID = "0000-0001-5226-3441")),
    person(given = "Ana-Teresa", family = "Maia",
    email = "maia.anateresa@gmail.com", role = "aut", 
    comment = c(ORCID = "0000-0002-0454-9207")),
    person("Centre for Biomedical Research, University of Algarve", role = c("cph", "fnd"))
    )
Description: gwasrapidd: GWAS R API Data Download.
    gwasrapidd provides easy access to the NHGRI-EBI GWAS Catalog data by
    accessing the REST API https://www.ebi.ac.uk/gwas/docs/api.
Depends:
    R (>= 3.5)
License: MIT + file LICENSE
URL: https://github.com/ramiromagno/gwasrapidd
BugReports: https://github.com/ramiromagno/gwasrapidd/issues
Encoding: UTF-8
Language: en-US
LazyData: true
RoxygenNote: 6.1.1
Imports:
    magrittr,
    httr,
    urltools,
    pingr,
    stringr,
    concatenate,
    dplyr,
    jsonlite,
    purrr,
    tibble,
    glue,
    tidyr,
    assertthat,
    rlang,
    methods,
    lubridate,
    plyr,
    testthat
Suggests:
    httptest,
    spelling,
    covr,
    knitr,
    rmarkdown
Collate:
    'class-associations.R'
    'class-studies.R'
    'class-traits.R'
    'class-variants.R'
    'data.R'
    'ebi_server.R'
    'generics.R'
    'get_associations.R'
    'get_metadata.R'
    'get_studies.R'
    'get_traits.R'
    'get_variants.R'
    'gwasrapidd-package.R'
    'list_joins.R'
    'missing.R'
    'parse-associations.R'
    'parse-studies.R'
    'parse-traits.R'
    'parse-utils.R'
    'parse-variants.R'
    'post-studies.R'
    'post-traits.R'
    'post-variants.R'
    'recursive_apply.R'
    'request.R'
    's4-utils.R'
    'sure.R'
    'tests.R'
    'utils-pipe.R'
    'utils.R'
VignetteBuilder: knitr
biocViews: ThirdPartyClient, BiomedicalInformatics, GenomeWideAssociation, SNP

[bioc-issue-bot]

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]

For the duration of the review process, please update the 'master' branch to contain only files to be included in the R package.

Please provide 'accessors' rather than using direct slot access @, e.g., in the vignette.

The assigned reviewer will provide a more comprehensive review.

[bioc-issue-bot]

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]

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

620a66e Version bump!

[bioc-issue-bot]

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]

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

1663ad3 Few more updates to comply with Bioc requirements.

[bioc-issue-bot]

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.

[ramiromagno]

Hi @mtmorgan:

Regarding the requirement: providing 'accessors' rather than using direct slot access @:

I am aware of the good practice of creating functions to access slots of S4 objects in general. Let me explain however why I think in my specific case it is more appropriate to use the slot access @ style rather than the setter/getter approach.

Here are the reasons:

• All S4 objects in this package are created by the package and never by the user. These objects are the result of web requests to a REST API server.
• The different ways the user can change these objects is via exported functions from the package itself. There is not a use case where the user is expected to change the value of a slot, in the sense of a setter function.
• In this package, all S4 objects are really just mini-, in-memory, relational databases, effectively a list of tibbles, i.e., each slot is a tibble. The only way the user is expected to interact with those slots is to get data from it. So the @-style of accessing slots is perfectly suited here. Creating a getter for each slot would simply be redundant. Moreover, the principle of separating interface from implementation does not really apply here as the slot names are the perfect interface: each slot name stands for a table in the S4 object database. If I ever need to change the name of these tables, or deprecate them, even if I had implemented getters, I would still have to change those getters (or slot names) anyway... My point is, implementation, here, and interface, are one and the same thing.

[bioc-issue-bot]

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

4d15e4f Removed .gitattributes.
d6f0e63 Moved .Rproj file to .gitignore.
7c9ef0a Version bump.

[mtmorgan]

I appreciate that the data is 'read only' and that interface and implementation overlap. Nonetheless, please provide accessors, if only to discourage your users from thinking that all objects are as simple as yours.

[bioc-issue-bot]

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

e1da607 Added function open_in_gwas_catalog().

[bioc-issue-bot]

Dear Package contributor,

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

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

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

Please see the build report for more details.

[ramiromagno]

@dvantwisk:

Only the Windows builld is failing because of a timeout at this step:

** running examples for arch 'i386' ... ERROR

I don't know why. I can only speculate: maybe that environment does not provide internet connection and those examples hang waiting for a response...

[dvantwisk]

Sorry for the delay. It's not immediately clear what is causing the package to timeout of that architecture. First, you might consider trying a version bump to see if the issue repeats itself in a new build. Rarely, the builds fail for reasons other than the package.

Given windows does lack certain features it may be the case that either your package or packages that you are using may not be able to run on windows.

If the issue persists it is possible to tell the builder to ignore windows, however, this is not ideal since the package can't be guaranteed to work on this platform.

[bioc-issue-bot]

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

7a074d7 Version bump for bioc build troubleshooting.

[ramiromagno]

Thanks @lshep! I am going to update the code.

[bioc-issue-bot]

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

34c745d Updated code in functions that use browseURL. The...

[bioc-issue-bot]

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]

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

4b6f459 Fixed spelling errors
0354322 Fixed doc links to POSIXct
c22d3a6 Fixed doc links to %>% The build on Windows was g...
0dcf922 Fixed doc links to gwasrapidd::union in gwasrapidd...
3203569 Version bump 0.99.7: doc links fixed (hopefully)

[bioc-issue-bot]

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.

[dvantwisk]

Here is a review of your package. I think the package looks very well written and I only have a few concerns. [REQUIRED] tags need to be addressed or the change need to be made. [CONSIDER] tags are changes that are advisable but not required. Please address these proposed changes and message me back when you are finished.

GENERAL

• [REQUIRED] Your package is 26MB in size. We tend to want to see our
  repositories 5MB in size or less. Is there anywhere that you can reduce
  the size of the package? If you deleted any large files and committed the
  change, these files may still appear in your .git directory. You can
  use the BFG-repo-cleaner https://rtyley.github.io/bfg-repo-cleaner/ to
  remove deleted file from your .git history.

R/GENERAL

• [REQUIRED] It seems that some of the generics and classes you are delcaring
  and exporting have very common names. Problems can aarise if two generics with
  the same name are declared in different packages and loaded at the same time.
  For this reason, I need to ask that you make the names of your generics less
  common or to use existing genrics from other pacakges. For example, you declare
  a union generic but there is already a union generic present in the
  IRanges package that has a similar declaration. You should be able to import
  and use this generic for your own purposes. For Class names like studies I
  would encourage you to use the name GWASstudies or something similar. Please
  correct the following symbols:
  Classes:

  • associations
  • studies
  • traits
  • varients

  Generics:

  • union
  • intersect
  • setdiff
  • setequal
  • bind
  • n

R/browser.R

• [CONSIDER] L41: In the open_in_gwas_catalog function, consider using
  match.arg() to declare gwas_catalog_entity. This would involve writing the
  delcaration as gwas_catalog_entity = c('study', 'varient', 'trait', etc..).
  The default argument is the first element in the vector, 'study'. The value
  that the user selects can be obtained using
  gwas_catalog_entity = match.arg(gwas_catalog_entity). This method is
  preferable because it allows the users to see possible values of
  gwas_catalog_entity when they print the function. Also, an error will be
  thrown if the user gives input outside of the allowed values. This can take the
  place of the error check in lines L50-69. This can be done in multiple other
  functions throughout the package.

R/sure.R

• [REQUIRED] It seems that the sure() function will only give a prompt during
  an interactive session. However, I would imagine that there are cases where the
  user would automatically want to answer "yes" instead of giving "yes" as input
  during an interactive prompt and it doesn't seem that this functionality exists.
  Can this functionality be added?

vignettes/gwasrapidd.Rmd

• [REQUIRED] There seems to be many uses of eval=FALSE here. Is there a reason
  why these are being used? We would like all user-facing code to be run by the
  build system, but recognize there are some cases that these can't be run in the
  vignette (such as L107 where it would presumably open a web browser).

[ramiromagno]

Hi @dvantwisk:

Thank you for your review!

Before I start making changes to the code let me know your thoughts on these points:

1. My package size is big because of the mock files used for testing. I have reduced the number of files and the size per file used before submitting here to Bioconductor. Nevertheless, I believe they are leaving their footprint in the .git folder. As I said early in this issue, I have tried bfg-repo-cleaner but to no avail. So I am afraid that to really reduce the package size I would need to start a new git repository... I guess this implies discarding the git history so far, so I am a bit reluctant...
2. I am happy to change the S4 class names to something more specific, and I am aware of the camelCaps/UpperCamelCase convention. But would it be ok to use something like: gwas_studies, gwas_associations, gwas_variants and gwas_traits? (I have a preference for lowercase, underscore-separated words -- I promise I will be consistent).
3. With respect to the names of the generics, I will change them. But I must confess I picked them precisely this way because I thought their semantics would be obvious if the user was already acquainted with these functions in R base and dplyr.
4. The suggestion on using the match.arg() function seems like a really nice idiom. Will do!
5. Actually the sure() function already allows for non-interactive use but I realize now that this is not very clear from the documentation (sure() is not exported). I will make it more explicit that the user can set the parameter default_answer to 'yes'/'no', and that, then, sure() proceeds without interruption using default_answer as answer.
6. On your remark about the eval=FALSE, I will revise those code chunks. There might have been a reason at a certain point for doing that but I agree with you that it needs to change. Only the line that launches the browser will be left with eval=FALSE.

[dvantwisk]

1. The package size should be fine, I just want to be sure you know that we would like packages to take up a small amount of space. If there's nothing else that you can remove, I think the size is okay.

2. Yes, using the gwas_* syntax is okay.

3. Most R base functions are implemented as S4 generics in the BiocGenerics package. I encourage you to write methods for these and dispatch them on your classes. For example, the following are present in BiocGenerics:

> library(BiocGenerics)
> getMethod(setdiff)
Method Definition (Class "derivedDefaultMethod"):

function (x, y, ...)
base::setdiff(x, y, ...)
<bytecode: 0x7fa7ff679ee0>
<environment: namespace:BiocGenerics>

Signatures:
        x
target  "ANY"
defined "ANY"

> getMethod(union)
Method Definition (Class "derivedDefaultMethod"):

function (x, y, ...)
base::union(x, y, ...)
<bytecode: 0x7fa7fd586790>
<environment: namespace:BiocGenerics>

Signatures:
        x
target  "ANY"
defined "ANY"

> getMethod(intersect)
Method Definition (Class "derivedDefaultMethod"):

function (x, y, ...)
base::intersect(x, y, ...)
<bytecode: 0x7fa7faa867d0>
<environment: namespace:BiocGenerics>

Signatures:
        x
target  "ANY"
defined "ANY

For dplyr methods, you are allowed to use delcare them as an s3 method. For example:

filter.gwas_study <- function(.data, ..., .preserve=FALSE)
{
 <your function>
}

4. Good to hear!

5. It wasn't clear that sure() allowed it, so the documentation change should be helpful.

6. It's likely that some of these eval=FALSEs were probably added to reduce the runtime on the windows machine that was timing out. Since it's no longer an issue, it doesn't seem that some of them are necessary.

[dvantwisk]

Hello,

Just touching base with you again. Have you or do you plan to make further changes to the package?

[ramiromagno]

Hi @dvantwisk:

I have not decided yet.

[bioc-issue-bot]

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

92fa9be Added is_mapped_gene to genomic contexts of varian...

[bioc-issue-bot]

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.

[dvantwisk]

Hi,
Just checking up on your progress with this issue. We generally like to see packages being actively worked on, or we close the issue within two weeks. Closed issues can be reopened when the package maintainer is ready to work on it again. Are you still actively working on correcting issues to this package?

[ramiromagno]

Hi @dvantwisk:

I think for the moment it is best to close this issue indeed.

Thank you for all the help though!

[bioc-issue-bot]

This issue is being closed because there has been no progress
for an extended period of time. You may reopen the issue when
you have the time to actively participate in the review /
submission process. Please also keep in mind that a package
accepted to Bioconductor requires a commitment on your part to
ongoing maintenance.

Thank you for your interest in Bioconductor.