Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

(inactive) CTD #1630

Closed lashmore closed 4 years ago

lashmore 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

Hi @lashmore

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: CTD
Title: CTD method for “connecting the dots” in weighted graphs
Version: 1.0.0
Date: 2017-05-25
Author: Lillian Thistlethwaite [aut, cre]
Maintainer: Lillian Thistlethwaite <lillian.thistlethwaite@bcm.edu>
Description: An R package for pattern discovery in weighted graphs. Two use cases are achieved: 1) Given a weighted graph and a subset of its nodes, do the nodes show significant connectedness? 2) Given a weighted graph and two subsets of its nodes, are the subsets close neighbors or distant?
Depends: R (>= 4.0), gmp, igraph, stats, grDevices, graphics
Suggests: 
    knitr,
    rmarkdown,
    huge,
    plotly,
    gplots,
    RColorBrewer,
    testthat
VignetteBuilder: rmarkdown
biocViews: BiomedicalInformatics, Metabolomics, SystemsBiology, GraphAndNetwork
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.1.1
mtmorgan commented 4 years ago

Please use version number 0.99.0 (if the package has not previously been released at version 1.0.0) or 1.99.0 (if it has been released at version 1.0.0).

Please use the Authors@R notation, and not Author: / Maintainer:

Please make sure that your source package passes BiocCheck::BiocCheck().

Please post a comment here when you have made these initial changes; the issue will then be re-opened and the review process started.

lashmore commented 4 years ago

@mtmorgan, I have updated the version and Authors content as described! I confirm package passes R CMD check and BiocCheck::BiocCheck().

bioc-issue-bot commented 4 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. It is required to push a version bump to git.bioconductor.org to trigger a new build.

Bioconductor utilized your github ssh-keys for git.bioconductor.org access. To manage keys and future access you may want to active your Bioconductor Git Credentials Account

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: "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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/CTD to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 4 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 2f3171ca014523333d736a27236c0781ff6f728d

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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/CTD to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

lashmore commented 4 years ago

Hello @mtmorgan and @LiNk-NY, I believe the Mac OSX build error reflects an absent system library "gmp", which is required for the gmp R package dependency to run. gmp is a system library for UNIX / Mac operating systems. The chooseZ() function is a gmp R package function, and the gmp R package depends on gmp (>= 4.2.3) being installed properly on the system. Can you verify whether the Mac build system has gmp installed and if so, which version?

Also, the WINDOWS build system seems to have skipped nearly everything with no debugging info. I tested it on my husband's Windows computer, and we determined that the Windows machine absolutely needs to have webshot and phantomjs installed. Without it, plotly has a hard time producing PDF output and the vignette build fails.

require(webshot) webshot::install_phantomjs()

LiNk-NY commented 4 years ago

Hi Lillian, @lashmore

Can you bump the package version for another build?

I believe the Mac OSX build error reflects an absent system library "gmp", which is required for the gmp R package dependency to run. gmp is a system library for UNIX / Mac operating systems. The chooseZ() function is a gmp R package function, and the gmp R package depends on gmp (>= 4.2.3) being installed properly on the system. Can you verify whether the Mac build system has gmp installed and if so, which version?

After investigation by Hervé @hpages, we realized that the binary installation of gmp is not currently working with the example code factorialZ(0:10) but it does work when the package is built from source.

Error message with binary installation on macOS 10.14.6 Mojave | x86_64 | x86_64-apple-darwin18.7.0 | 4.0.2 (2020-06-22) -- "Taking Off Again":

> library(gmp)
Attaching package: 'gmp'
The following objects are masked from 'package:base':
    %*%, apply, crossprod, matrix, tcrossprod
> factorialZ(0:10)
 *** caught illegal operation ***
address 0x1087a13fb, cause 'illegal opcode'
Traceback:
 1: factorialZ(0:10)
Possible actions:
1: abort (with core dump, if enabled)
2: normal R exit
3: exit R without saving workspace
4: exit R saving workspace
Selection: 

In the meantime, you should contact the maintainer and CRAN to notify them of this issue with the gmp package.

If possible, avoid using the factorialZ / choozeZ function in your package code to avoid this gmp package dependency.

Best, Marcel

bioc-issue-bot commented 4 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 3e21c82d99e6d996a71828e8d45e1265cbf543cb

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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/CTD to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

lashmore commented 4 years ago

Hello @LiNk-NY, it's great to hear you guys were able to identify issues with the gmp installation which explains the error! Unfortunately, choozeZ() is very important for our mle.getEncodingLength() function when the problem is applied to large outcome spaces, so it is needed. I will reach out to CRAN and the gmp package maintainers to let them know.

The newest build report skipped both the Mac and Windows builds this time. From what I can tell, Mac was skipped because of the chooseZ() error, but Windows being skipped wasn't clear what was occurring. I did see the "get_dcf_info failed" message in the build tokay1 (Windows) Summary, but the read.dcf() function call from devtools works just fine. There isn't much else diagnostic information I can use besides this. How are these undiagnostic SKIPS typically debugged?

bioc-issue-bot commented 4 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: c22cf9d12919c6c43e0bcd0378aefe24226ea715

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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/CTD to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

lashmore commented 4 years ago

Hello @LiNk-NY, just wanted to make sure you saw my comment prior to the last push change I made which didn't change the outcome.

LiNk-NY commented 4 years ago

Hello @LiNk-NY, it's great to hear you guys were able to identify issues with the gmp installation which explains the error! Unfortunately, choozeZ() is very important for our mle.getEncodingLength() function when the problem is applied to large outcome spaces, so it is needed. I will reach out to CRAN and the gmp package maintainers to let them know.

Hi Lillian, @lashmore

We will have to wait and see for the resolution of the gmp package status. I had a quick look at your package and saw that it does not make any attempt to integrate with Bioconductor despite having functions (e.g., data.zscoreData) that could potentially take a SummarizedExperiment as input.

There are other packages that may provide useful data representations (e.g., mzR; see https://www.bioconductor.org/packages/release/BiocViews.html#___Metabolomics for more). Note, I provide metabolomics packages based on the biocViews for the current package.

It would be great to have some kind of integration with Bioconductor, otherwise, it may be better on CRAN.

The newest build report skipped both the Mac and Windows builds this time. From what I can tell, Mac was skipped because of the chooseZ() error, but Windows being skipped wasn't clear what was occurring. I did see the "get_dcf_info failed" message in the build tokay1 (Windows) Summary, but the read.dcf() function call from devtools works just fine. There isn't much else diagnostic information I can use besides this. How are these undiagnostic SKIPS typically debugged?

I would rely on base::read.dcf rather than what is in devtools. I don't have Windows currently to check this.

Best, Marcel

bioc-issue-bot commented 4 years ago

Received a valid push on git.bioconductor.org; starting a build for commit id: c66597170f527acd1d578d9fb7f2c37ee841f963

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: "ERROR, skipped". 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. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/CTD to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

lashmore commented 4 years ago

A member from the Bioconductor list serv fixed the Windows build error - it had to do with quotation use in the DESCRIPTION file. Windows builds and runs just fine now. All that is left is the gmp R package issue. I will reach out to the maintainer again.

Importantly, when I pass the Mac build and checks, will you require that I modify CTD to integrate Bioconductor's data representations in order for CTD to be accepted into Bioconductor? I've already changed my codebase a lot to pass the BiocCheck tests, and I would have preferred to know about these requirements prior to submitting to Bioconductor. These are not listed or linked on the guidelines for submissions.

Yes, CRAN has a faster package acceptance rate than Bioconductor, but Bioconductor's findability is reportedly better than CRAN and the software packages submitted to Bioconductor is more targeted for software solutions for "-omics" research. Your suggestion for me to submit to CRAN reminds me of a scene from Mean Girls: https://images.app.goo.gl/ke6ByUm2LtUL5kEB8.

lashmore commented 4 years ago

@LiNk-NY , here's what the package maintainer for R gmp said about the binary issue:

My package depends on a c++ library (libgmp). It can be build withm any version of libgmp, if its version is >= 5.0. I suppose cran binary is done on a version, and your system have another version. This is more - I think - a general purpose on any R package that depends to a system library. I have no solution for this.

Can you tell me what version of libgmp is installed on the Mac build system?

LiNk-NY commented 4 years ago

Hi Lillian, @lashmore

These are not listed or linked on the guidelines for submissions.

http://bioconductor.org/developers/package-guidelines/#rcode Point number 4 states that developers should make use of appropriate existing packages in Bioconductor. Our package submission page also states this in the introduction: https://bioconductor.org/developers/package-submission/

Interoperate with other Bioconductor packages by re-using common data structures (see S4 classes and methods) and existing infrastructure (e.g., rtracklayer::import() for input of common genomic files).

This is one of the more emphasized points across the Bioconductor review process in order to make the user experience favorable across packages.

If interested, here is an outline of recommended classes for re-use: https://bioconductor.org/developers/how-to/commonMethodsAndClasses/

I did not mean to turn you away like the Mean Girls GIF :sweat_smile: but rather to provide some feedback as to how the current package can be improved to work well with the rest of Bioconductor. You are more than welcome to update it and we'll be happy to review it.

We are looking into the libgmp version on the Mac builder.

Best, Marcel

LiNk-NY commented 4 years ago

UPDATE Hi Lillian, @lashmore

I've spoken to one of the maintainers of the build system and they said that we do not install libgmp on the builders and that we rely on the installation gmp package binary as most Mac users do. We installed from source as a workaround to the binary being broken. As we see it, there are mostly two options:

1) convince the gmp and/or CRAN folks that the current gmp Mac binary needs a fix (and update it) 2) have the CTD maintainer avoid the gmp package dependency all together

Best, Marcel

lashmore commented 4 years ago

Hey, @LiNk-NY , thanks for clarifying everything for me, especially your comment about the Mean Girls meme, haha!

I've had a long think about all of this and I have decided I'm really not comfortable adding more dependencies to my R package on principle, just to be more like other Bioconductor packages. The best thing about my codebase is that it is simple and fast, and needs very little to function.

The gmp dependency is an unfortunate exception, and it seems we have reached an impass about how to resolve this issue as well. I cannot remove the dependency, as it is required for computation on large outcomes spaces, and I cannot control the moderator who does not seem interested in taking ownership of this installation issue, when he cannot replicate the issue on his own.

So long story short, I am going to try my hat out with CRAN and see if I can't move forward with them instead. I will consider Bioconductor for future projects and packages I inevitably develop and I thank you for your time and consideration.

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