search

Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

(inactive) granulator #1317

Closed BioVinci closed 4 years ago

BioVinci commented 4 years ago

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 am familiar with the essential aspects of Bioconductor software management, including:

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

bioc-issue-bot commented 4 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 build report for more details.

BioVinci commented 4 years ago

Hi @hpages

Thank you for the feedback on the package. I tried to address all the issues you pointed out.

  1. In the function deconvolute() I apply the function DeconRNASeq() from the package DeconRNASeq. The printed output of that function caused the verbosity. I already wrapped the call to the function DeconRNASeq() in SuppressMessages() and SuppressWarnings(), but as you pointed out, there is still a lot printed to the console during the execution. I therefore used invisible(capture.output(x<-function())) to suppress the print statements.

Further deconvolute() calls e1071::svm() which causes the Warning: reaching max. number of iterations. In deconvolute() I compare multiple forms of linear regression, however, smv() does not converge. As far as I know, there is not option to set the limit of iterations in the svm() function. Shall I simply supress the warning?

  1. importFrom grDevices colorRampPalette added to Namespace

  2. BiocCheck produces notes regarding number of characters per row (>80) and indents (multiples of 4 spaces). I reduced both to less than 5% in the overall code. Further, there is a note regarding my subscription to the mailinglist, where I cannot figure why this note occurs. I subscribed to the list.

  3. Strangely, when I knit the vignette to an html_vignette output, the table 1 renders properly.

  4. = assignemts are converted to <-

  5. README.md is in sync again with the current version of the package.

  6. vignette.html is removed from directory

Thank you very much for your help! Best, Vince

bioc-issue-bot commented 4 years ago

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

79dad43 initiate version bump

bioc-issue-bot commented 4 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 build report for more details.

BioVinci commented 4 years ago

Hi @hpages

There seems to be a problem with initating a version bump. I reached 0.99.99. When extending the version numbering to 0.999.1 for example biocCheck returns an error. Is there anything I could do to reset the labeling?

Apologies for the inconvenience.

mtmorgan commented 4 years ago

use 0.99.100 0.99.101, ...

bioc-issue-bot commented 4 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 build report for more details.

hpages commented 4 years ago

Thanks again for the improvements. Some problems remain. See below.

Thanks, H.

Item 1: It's unfortunate that you have to use hacks to work around flaws in the implementations of functions like DeconRNASeq::DeconRNASeq() and e1071::svm(). Ideally these problems should be addressed upstream though. However that will only happen if someone takes the time to report them to their authors. Something you might want to consider at some point.

Item 3: You've reduced the number of notes reported by R CMD BiocCheck from 10 to 9 after I complained that 10 notes was a lot, and then from 9 to 8 after I complained again that there was still too many notes. So it sounds like I need to be more explicit if we want to make more progress more quickly on this. How about you reduce to 6 notes (or less). I suggest you start by looking at the suggestion of using vapply() instead of sapply(). Bioconductor cares about code quality and readability. The purpose of these notes is to help you improve that.

Item 4: For the table in the vignette, I don't know what commands you use exactly to knit the vignette to HTML but the vignette that will end up in the source tarball and on the user machine is the one that matters. To see this one, build the source tarball with R CMD build, install it, then start R, do browseVignettes("granulator") and click on the URL. This will open the HTML vignette in your browser. The vignette I see when doing this has a table that is unreadable because not formatted as a table.

Item 6: It doesn't seem that your README.md file is really in sync again with the current version of the package. For example the list of attached packages displayed by sessionInfo() doesn't reflect what the real list is with the current granulator. But a more serious problem is that doing deconvoluted$coefficients$rl_model_sigMatrix_ABIS[1:5, 1:5] in a real session returns NULL and not the matrix that you're showing in your README.md. I've tried to suggest earlier that this kind of static document is a really bad idea as they are almost always out-of-sync with the current state of the package. This is why we have dynamic vignettes! They avoid that problem. So it makes no sense to reintroduce the problem by adding a document that is redundant with the vignette but that almost surely contains broken code and/or inaccurate output (or will again in the very near future).

Other things:

  1. grDevices needs to be listed in the Imports field of your DESCRIPTION file (as reported by R CMD BiocCheck). It also seems that a few packages could move from Depends to Imports (e.g. broom, epiR, magrittr, and maybe others). This would reduce the number of packages that get attached to the search path when granulator is loaded with library(granulator).

  2. The plotting functions (e.g. plt.benchmark) are poorly named (AFAICR I've never seen a software where plt is used as short for plot). Also this naming style is inconsistent with the other functions in the package where the underscore is used as separator (e.g. benchmark_methods and correlation_analysis). Please rename them (e.g. to plot_benchmark) so the names are readable and have a style that is consistent with your other functions.

bioc-issue-bot commented 4 years ago

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

07f7714 adress issues raised by BiocCheck

bioc-issue-bot commented 4 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 build report for more details.

bioc-issue-bot commented 4 years ago

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

4e72108 adress issues raised by BiocCheck

bioc-issue-bot commented 4 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 build report for more details.

BioVinci commented 4 years ago

Hi @hpages ,

Thank you for your comments. I tried to resolve as many issues as possible, but there are a few notes, where I currently do not find a way to proceed.

Thank you for your help.

bioc-issue-bot commented 4 years ago

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

8c2c04c adress issues raised by BiocCheck

bioc-issue-bot commented 4 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 build report for more details.

hpages commented 4 years ago

Thanks, I will review you latest changes.

One thing that is worrying me though is that you don't seem to understand software versioning. The current version of your package is 0.99.003. Unlike with decimal numbers where the dot marks the beginning of the fractional part, the dots in a version number like x.y.z is used as a separator between whole numbers x, y, and z. So it doesn't matter how man zeroes you use to prefix these whole numbers, these zeroes will be ignored by any tool that needs to perform version comparison. In other words, 0.99.003 is semantically equivalent to 0.99.3 and is considered a lower version than 0.99.5 which was the version of your package 5 months ago!

After you reached 0.99.99 one month ago, @mtmorgan said you should just keep going with 0.99.100, 0.99.101, etc... As you can see the pattern is very simple: just bump z (i.e. increase it by 1) each time you need to bump the version of your package. No need to prefix anything with zeroes.

It's important that you get this right before acceptance of your package. Proper package versioning is a key aspect of good software management and a required skill for any Bioconductor package developer/maintainer.

Thanks!

bioc-issue-bot commented 4 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: "skipped, 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.

hpages commented 4 years ago

Hi @BioVinci ,

Item 4:

The references in the table were not properly displayed. The problem occurred because the title of the package exceeded a single line. Shortening the title to a single line resolved the issue.

I'm not sure we are talking about the same thing. The problem of Table 1's formatting being broken in the HTML vignette remains.

Item 6:

You mentioned earlier, that the README file is static, since I copy/paste the ouput of commands. Could you point out where exaclty this is the case?

The entire document is a repetition of the vignette. It's not an exact copy though and the code in it is broken (as mentioned earlier). This is the fate of code included in static documents, unfortunately.

Item 8: Thanks for moving broom, epiR, and magrittr fron Depends to Imports. Note that these are the 3 packages I mentioned as examples of packages that are candidates for such a move but I'm sure there are others. I was hoping that you would do your part to determinate what the others are.

One more thing:

Item 10: The benchmark_methods() function is way too big which makes it hard to read. And the fact that the body of the functions defined inside it are at the same indentation level as the main body doesn't help.

Thanks!

bioc-issue-bot commented 4 years ago

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 interest in Bioconductor.