BgeeCall_review.md

BgeeCall review

BiocCheck

• Basic run of BiocCheck('BgeeCall') on the package

    * ERROR: Packages providing 1 object(s) used in this package should
    be imported in the NAMESPACE file, otherwise packages importing
    this package may fail.
  
    package::object in function()
    graphics::mtext in plot_distributions()
  

• File size too big, software packages need to be 5MB in size or less. Please check "BFG cleaner".

    * Checking individual file sizes...
    	* WARNING: The following files are over 5MB in size:
    '.git/objects/pack/pack-94ca9deb2be0a9e6fc630aa9c82ad98cb3dfbb24.pack'
  

• Other issues,

* Checking for valid maintainer...
    * WARNING: Use Authors@R (preferred) or Author/Maintainer fields
      not both.
* Checking DESCRIPTION/NAMESPACE consistency...
    * WARNING: Import BgeeDB in NAMESPACE as well as DESCRIPTION.
* Checking vignette directory...
    This is a software package
    * WARNING: Vignette[s] still using 'VignetteIndexEntry{Vignette
      Title}' Update the following files from using template values:
        bgeecall-manual.Rmd

• Coding practice issues,

* Checking coding practice...
    * NOTE: Avoid 1:...; use seq_len() or seq_along()
      Found in files:
        retrieveFromCommunity.R (line 27, column 27)
        retrieveFromCommunity.R (line 44, column 31)
		
* Checking man page documentation...
    * NOTE: Consider adding runnable examples to the following man
      pages which document exported objects:
      create_kallisto_index.Rd, generate_calls_workflow.Rd,

Please run BiocCheck('BgeeCall') on the package and make the appropriate changes. Install the package BiocCheck and run the function. We cannot make the change without fixing the issues given above.

You end up getting,

Summary:
ERROR count: 1
WARNING count: 5
NOTE count: 8

R CMD build

ok

R CMD INSTALL

ok

Why not use the usual channel for this review?

Hi @hpages

So, the BgeeCall package was a workflow package which is being ported over as a software package.

There was a discussion about this a few days ago, and @mtmorgan can comment more maybe, but we decided to not go through the deprecation cycle for the workflow package and just review it for what it is as a software package.

I'm aware of this but still don't see why the usual channel wasn't used for this. Are you saying the package is not on GitHub so can't be submitted to the issue tracker? This not only provides convenience and transparency to the review process (automated builds on all platforms, with public build reports, use of labels to show status) but it also avoids displacing the process outside the Bioconductor space.

Hello Nitesh,

I fixed some BiocCheck issues.

I still have one WARNING because my .pack file is bigger than 5Mo (it is actually 5.8Mo). It was already the case during the last review of my package. Is it ok for you if I keep it like that? I used bfg to remove all blob files bigger than 5Mo.

Remaining NOTES are :

[1] "Recommended function length <= 50 lines."
=> I have one big function of 120 lines. and 10 function between 50 and 76 lines. Not sure it is better to split them in different functions. Please tell me if you want me to work on this.
[2] "Consider adding runnable examples to the following man pages which document exported objects:"
=> The package is dependant of kallisto and these functions can not be run without kallisto. As it is not allowed to install kallisto by default user can choose to install it using a function but then I can not test 2 functions using kallisto (I added examples with dontrun{} tags)
[3] "Usage of dontrun{} / donttest{} found in man page examples."
=> correspond to examples of functions dependant of kallisto
[4] "Consider shorter lines; 235 lines (4%) are > 80 characters long."
=> I try to avoid long lines. These lines should not be longer than 100 characters
[5] "Consider multiples of 4 spaces for line indents, 61 lines(1%) are not."
=> do I need to take care of these lines?
[6] "Cannot determine whether maintainer is subscribed to the bioc-devel mailing\nlist (requires admin credentials). Subscribe here:\nhttps://stat.ethz.ch/mailman/listinfo/bioc-devel"
=> I am subscribed to the bioc-devel mailing list.

Hope my modifications/comments helped for the review process.

and... Merry Christmas :-D

Hi @jwollbret

Thanks for getting back to me on this.

I'm sure you are subscribed to bioc-devel.

I will take a look at this today and get back to you. I think we should skip the SPB and just review the package behind the scenes and get the package converted from workflow to software.

Hello Julien,

A few of things,

1. You need to update your Bioconductor repo as well. We'll just use that to check your package and code for review. I need know which repo I should review, because last time I just reviewed the Bioconductor Git version of your package.

2. Pay attention to the version numbers. Your GitHub repo is not in sync with your Bioconductor git repo. Make sure the devel branch is updated with an appropriate version number 1.3.0 (It has to be odd). It needs the commits which were made by the Bioconductor Core team.

3. Errors your package on Github generates,

* Checking version number validity...
    * WARNING: y of x.y.z version should be odd in devel
* Checking R Version dependency...
    * WARNING: Update R version dependency from 3.6.0 to 4.0.

4. There is still one .pack file greater than 5MB

 Checking individual file sizes...
    * WARNING: The following files are over 5MB in size:
      '.git/objects/pack/pack-520b3f763f0b30173242593c976261b30f7b946c.pack'

5. Please check the way you throw warnings,

End-User messages
• message() communicates diagnostic messages (e.g., progress during lengthy computations) during code evaluation.
• warning() communicates unusual situations handled by your code.
• stop() indicates an error condition.
• cat() or print() are used only when displaying an object to the user, e.g., in a show method.

Just throw a warning(), don't imitate it with cat(paste0())

                cat(
                    paste0(
                        "\nWARNING: BgeeCall could not access
intergenic releases information from the internet, but a release
information file was found in the download directory ",
                        getwd(),
                        ". This release file will be used, but be warned that it
may not be up to date!\n"
                    )

When you use stop()

> stop()
Error:

So, you don't need to write "ERROR"

                stop(
                    "ERROR: BgeeCall could not access Bgee
intergenic releases information. Is your internet connection working?"
                )

Your code will print out Error:ERROR.....

6. In the tryCatch function you use "stop" in the warning block, there is an error argument

warning = function(w) {
            stop("kallisto is not installed. You should either
    - automatically install a version of kallisto used only by this package (see vignette for more detai\
ls)
    - install kallisto on your system following official website instructions (https://pachterlab.github\
.io/kallisto/download)")

eg: tryCatch(stop(e), error = function(e) e, finally = print("Hello"))

7. When calling Kallisto please use system2 instead of the system calls. It is more robust. Do this all across your package.

8. Instead of this, if (removeFile == TRUE) you can just say if(removeFile). It will evaluate the logical operation.

9. This code won't evaluate, you want the spelling "Darwin"

get_os <- function() {
    sysinf <- Sys.info()
    if (!is.null(sysinf)) {
        os <- sysinf["sysname"]
        if (os == "Darwine") {
            os <- "osx"
        }

Please make sure the comments i've provided are corrected package wide if there are multiple instances of the same issue.

Thanks,

Nitesh

Hello Nitesh,

First of all happy new year.

Thanks a lot for all your comments.

For the point 4 :

Checking individual file sizes...
* WARNING: The following files are over 5MB in size:
'.git/objects/pack/pack-520b3f763f0b30173242593c976261b30f7b946c.pack'

The file is 5.8Mb. Could it be possible to keep it like that?

Could you please tell me on which Bioconductor branch I should push the update?
I tried to push it to the RELEASE_3_10 branch but I had an error message :
Error: Illegal version bump from '1.2.0' to '1.3.0'. Check http://bioconductor.org/developers/how-to/version-numbering/ for details

I think I took into consideration all comments (except comment 4).

Regards,

Julien

I merged last modifications of my package in the bioconductor master branch.

Hi @jwollbrett

I have reviewed your package and it seems good to be accepted. I've accepted officially and it is part of the software manifest now.

I've also updated the DESCRIPTION file to bump the version to 1.3.1. You should be able to see a build report tomorrow most likely, or maybe the day after at http://bioconductor.org/checkResults/devel/bioc-LATEST/.

So, please sync your Github repo with the Bioconductor git repo. You should be good to go for continuous development. If you have questions about how to maintain your package, please check http://bioconductor.org/developers/how-to/git/.

Best

Nitesh