Skip to content
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

Bioconductor reviewer comments #23

Closed
mmulvahill opened this issue Sep 13, 2017 · 3 comments
Closed

Bioconductor reviewer comments #23

mmulvahill opened this issue Sep 13, 2017 · 3 comments

Comments

@mmulvahill
Copy link
Contributor

mmulvahill commented Sep 13, 2017

Initial comment:

R1: Obviously needs better integration with Bioconductor.

Me: A little more explanation or an example of how would be greatly appreciated.

R1: We typically expect packages to plug into upstream and downstream workflows. The
vignette shows many ways of retrieving miRNA annotations, etc, but does not give an example of
using them in an actual analysis.

  1. Add an example using other bioconductor packages

R2: Hi,

I've done a first review. A few suggestions and comments below.

(1) BiocCheck output:

Checking for bioc-devel mailing list subscription...
ERROR: Maintainer must subscribe to the bioc-devel mailing list.
Subscribe here: https://stat.ethz.ch/mailman/listinfo/bioc-devel

  1. DONE Subscribe to mailing list

(2) Packages in 'Imports' should be fully or partially imported in NAMESPACE.

  1. DONE Look into 'imports' and NAMESPACE

(3) Internal functions

If you don't expect the user to call a function I would not export it. If the function is exported you need a full man page with examples. For example, add.multimir.links.Rd does not have an example.

  1. DONE Reduce number of exported functions

(4) Function names:

Some have underscores, some have dots ("."). It looks like you're using dot for internal functions. If a user can call a function it technically isn't internal. I'd recommend being consistent and using underscores for all exported functions.

  1. DONE Replace dots with underscores in exported functions

(5) Since this package is serving up annotations, they should support the same methods as the other annotations in Bioconductor. Please implement the select() family interface from AnnotationDbi on the objects you return from get.multimir(). The family includes select(), keys(), keytypes() and cols().

  1. Look into adding AnnotationDbi::select(), keys(), keytypes(), and cols() for get.mulitmir() objects

(6) Consider using the AnnotationFilter package for filtering of the results. If you feel you can't implement the filter interface in this package please explain why. The package is fairly new and if there are suggestions to expand compatibility then we'd like to hear them.

  1. Look at AnnotationFilter for filtering results

Let me know if you have questions.

@mmulvahill
Copy link
Contributor Author

0, 5, and 6 are larger tasks -- start there then fix other issues.

@mmulvahill
Copy link
Contributor Author

Fixed 2. in commit 50e78d4 and Fixed 4. in 50e78d4, 50e78d4, and 50e78d4. 3 previously fixed -- still a lot of exported functions, but all used somewhere or simple helpful utility functions.

@mmulvahill
Copy link
Contributor Author

All resolved! Package on Bioconductor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant