(inactive) retnfit - Githubissues

hstern2 commented 7 years ago

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

Repository: https://github.com/hstern2/retnfit

Confirm the following by editing each check box to '[x]'

[X] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
[X] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
[X] 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.
[X] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
[X] 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:

[X] The 'devel' branch for new packages and features.
[X] The stable 'release' branch, made available every six months, for bug fixes.
[X] Bioconductor version control using Subversion (optionally via GitHub).

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

bioc-issue-bot commented 7 years ago

Hi @hstern2

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: retnfit
Type: Package
Title: Fitting ternary network models using replica exchange Monte Carl
Version: 0.99.0
Date: 2017-03-04
Author: Harry A. Stern <hstern2@ur.rochester.edu>, 
    Matthew N. McCall <mccallm@gmail.com>
Maintainer: Harry A. Stern <hstern2@ur.rochester.edu>
Description: This package provides an implementation of the 
    replica exchange Monte Carlo algorithm for fitting 
    ternary network models of gene regulatory networks.
License: GPL (>= 2)
Imports: Rmpi, snow
NeedsCompilation: yes
biocViews: Software, CellBiology, GraphAndNetwork, GeneTarget, Network
Packaged: 2017-04-12 18:35:38 UTC; hstern2

mtmorgan commented 7 years ago

Before moderating this package so that it can be built for a comprehensive review, please update the vignette to be 'real', i.e., with evaluated code chunks rather than cut-and-paste results. Please also consider how this package fits in with other packages in the Bioconductor ecosystem, and introduces these packages either for data representation (e.g., SummarizedExperiment, GRanges) in the body of the package (via Imports: or perhaps Depends:), manipulation (e.g., BiocParallel, rather than Rmpi) or illustration (in the vignette).

hstern2 commented 7 years ago

thanks for the response.

1) I am now using BiocParallel (SummarizedExperiment and GRanges) aren’t applicable to this work however)

2) I have tried to include code to be evaluated in the \examples section of man/parallelFit.Rd but ran into trouble with R CMD check - I think the problem is that because this uses MPI, it requires R to be invoked with mpirun. is there a way to tell R CMD check to do that? I wasn’t sure this was possible because the Rmpi library itself seems to bypass the problem with \donttest{ (that’s the “solution” I chose)

thanks for any suggestions!

best - Harry

Harry Stern Center for Integrated Research Computing Wegmans Hall 1205, University of Rochester hstern2@ur.rochester.edumailto:hstern2@ur.rochester.edu (585) 275-8804

On May 23, 2017, at 12:43 PM, Martin Morgan notifications@github.com<mailto:notifications@github.com> wrote:

Before moderating this package so that it can be built for a comprehensive review, please update the vignette to be 'real', i.e., with evaluated code chunks rather than cut-and-paste results. Please also consider how this package fits in with other packages in the Bioconductor ecosystem, and introduces these packages either for data representation (e.g., SummarizedExperiment, GRanges) in the body of the package (via Imports: or perhaps Depends:), manipulation (e.g., BiocParallel, rather than Rmpi) or illustration (in the vignette).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Bioconductor_Contributions_issues_376-23issuecomment-2D303461814&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=tcmWcCMCGznNBozLkg3N7of4XQ7E_D_DqHrsqd5P3X4&s=WeeYGWzOQqIAHgrkBfoTB5xJ7SfFJbAXDcqZdoAQMtM&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ASdGsj9sRJIEvWPr2ddK6dsh4b5NK1G-5Fks5r8wyggaJpZM4Nj7yB&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=tcmWcCMCGznNBozLkg3N7of4XQ7E_D_DqHrsqd5P3X4&s=iTVW7w_pZo2gPj6tdpjGU-BV4xsptwG5j3cNuijQ63c&e=.

hstern2 commented 7 years ago

I commited changes to git repo. the example in man/parallelFit.Rd is no longer commented out with \dontest so this is a “real” example.

I still have trouble running R CMD check but it works with —no-examples

R CMD BiocCheck passes.

Thanks for your help getting this package submitted to bioconductor.

Harry

On May 25, 2017, at 11:57 AM, Stern, Harry harry.stern@rochester.edu<mailto:harry.stern@rochester.edu> wrote:

thanks for the response.

1) I am now using BiocParallel (SummarizedExperiment and GRanges) aren’t applicable to this work however)

2) I have tried to include code to be evaluated in the \examples section of man/parallelFit.Rd but ran into trouble with R CMD check - I think the problem is that because this uses MPI, it requires R to be invoked with mpirun. is there a way to tell R CMD check to do that? I wasn’t sure this was possible because the Rmpi library itself seems to bypass the problem with \donttest{ (that’s the “solution” I chose)

thanks for any suggestions!

best - Harry

Harry Stern Center for Integrated Research Computing Wegmans Hall 1205, University of Rochester hstern2@ur.rochester.edumailto:hstern2@ur.rochester.edu (585) 275-8804

On May 23, 2017, at 12:43 PM, Martin Morgan notifications@github.com<mailto:notifications@github.com> wrote:

Before moderating this package so that it can be built for a comprehensive review, please update the vignette to be 'real', i.e., with evaluated code chunks rather than cut-and-paste results. Please also consider how this package fits in with other packages in the Bioconductor ecosystem, and introduces these packages either for data representation (e.g., SummarizedExperiment, GRanges) in the body of the package (via Imports: or perhaps Depends:), manipulation (e.g., BiocParallel, rather than Rmpi) or illustration (in the vignette).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Bioconductor_Contributions_issues_376-23issuecomment-2D303461814&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=tcmWcCMCGznNBozLkg3N7of4XQ7E_D_DqHrsqd5P3X4&s=WeeYGWzOQqIAHgrkBfoTB5xJ7SfFJbAXDcqZdoAQMtM&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ASdGsj9sRJIEvWPr2ddK6dsh4b5NK1G-5Fks5r8wyggaJpZM4Nj7yB&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=tcmWcCMCGznNBozLkg3N7of4XQ7E_D_DqHrsqd5P3X4&s=iTVW7w_pZo2gPj6tdpjGU-BV4xsptwG5j3cNuijQ63c&e=.

mtmorgan commented 7 years ago

The currently blocking issue is the vignette, where the 'verbatim' commands need to be replaced with code chunks <<>>= ... @ that are actually evaluated. You'll need to work through the MPI issues on your end -- the build system will not launch your builds under mpi. You should (a) carefully examine the code that uses MPI and confirm that it is written efficiently (poor R code, typically 'iteration' via for() or lapply() can perform 100x slower than vectorized code) (b) ensure that the 'default' mode of evaluation is serial mode and (c) provide parallel evaluation as a user-configurable option.

mtmorgan commented 7 years ago

I am closing this issue. If you develop the vignette further, please update your git repository and let me know; I will then re-open the issue.

hstern2 commented 6 years ago

hi, I have finally updated this package with a configure script to detect MPI and build serial version if not there, use BiocParallel, have working vignette, etc

Harry Stern Center for Integrated Research Computing Wegmans Hall 1205, University of Rochester hstern2@ur.rochester.edumailto:hstern2@ur.rochester.edu (585) 275-8804

On Jul 9, 2017, at 6:35 AM, Martin Morgan notifications@github.com<mailto:notifications@github.com> wrote:

I am closing this issue. If you develop the vignette further, please update your git repository and let me know; I will then re-open the issue.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Bioconductor_Contributions_issues_376-23issuecomment-2D313911958&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=fX9rTPjTkgFhCAc4xxweTqKauY19i6IL4fjA8mRX-t0&s=HVsXNF_3HzYOe5xKXTGNcE6mPiXea-gF5JFbZmnF0GY&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ASdGstftg7PoWGO0TMfshV8FLmH6FRvqks5sMKzigaJpZM4Nj7yB&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=fX9rTPjTkgFhCAc4xxweTqKauY19i6IL4fjA8mRX-t0&s=tENwifjG5dRZ1T6Q65lJ2O_sdTh-5_31UKX72lBUUsM&e=.

mtmorgan commented 6 years ago

I didn't see your comment, and am opening this issue and assigning it to a reviewer.

bioc-issue-bot commented 6 years ago

Your package has been approved for building. Your package is now submitted to our queue.

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.

bioc-issue-bot commented 6 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, TIMEOUT, 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 6 years ago

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

461a6a0 version bump

bioc-issue-bot commented 6 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, TIMEOUT, 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 6 years ago

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

8161654 examples do not read from separate csv file

bioc-issue-bot commented 6 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, TIMEOUT, 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 6 years ago

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

d45c509 test for OpenMPI major version 1

bioc-issue-bot commented 6 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, 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.

hstern2 commented 6 years ago

thanks again for your help. one thing I wanted to check on was that I was subscribed to the bioc-devel mailing list - I am seeing

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

I tried subscribing at the website a couple times but I am not sure if it took.

Harry Stern hstern2@ur.rochester.edumailto:hstern2@ur.rochester.edu

On Jan 8, 2018, at 10:46 AM, Martin Morgan notifications@github.com<mailto:notifications@github.com> wrote:

I didn't see your comment, and am opening this issue and assigning it to a reviewer.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Bioconductor_Contributions_issues_376-23issuecomment-2D356003483&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=m7rPPxgPto9MmH8jok2AWRyLmRmnudBni540wHUa9-0&s=t-pKri8l4WdtRbliDbjXPhtsmhICbvC1dO_7OoVPZOI&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ASdGspHdOSBr3mptlxNdqDhM1gpAiMJ4ks5tIjhEgaJpZM4Nj7yB&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=m7rPPxgPto9MmH8jok2AWRyLmRmnudBni540wHUa9-0&s=NhomqnbaIx5jnQRFPSZELJ7ofcj_elqsQAMSaxL4q6g&e=.

mtmorgan commented 6 years ago

@hstern2 After entering subscription information the mailing list usual sends a confirmation email; this often ends up in people's 'spam' mail box...

hstern2 commented 6 years ago

yes that is exactly what happened. sorry for trouble

On Jan 9, 2018, at 5:06 PM, Martin Morgan notifications@github.com<mailto:notifications@github.com> wrote:

After entering subscription information the mailing list usual sends a confirmation email; this often ends up in people's 'spam' mail box...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Bioconductor_Contributions_issues_376-23issuecomment-2D356429615&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=203Q6l1TGjj1NjjvOg3sLzOgm8sIhf586ONJfIhVh8M&s=_uL13j-rXq0YKit3F5UneUkWUkWSV1p1g6Kg8AUr1Cc&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ASdGsu-5Fapq04XDZ7s2raVCVlNsJFO4qUks5tI-2DLbgaJpZM4Nj7yB&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=203Q6l1TGjj1NjjvOg3sLzOgm8sIhf586ONJfIhVh8M&s=rXLG198Fl0huWauG7kPciEQ2an28n4uGjJ6kNsHX_Vw&e=.

bioc-issue-bot commented 6 years ago

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

1f6ae72 version bump

bioc-issue-bot commented 6 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 6 years ago

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

8c81e37 remove inst/doc

bioc-issue-bot commented 6 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 6 years ago

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

96d7e8d print messages

bioc-issue-bot commented 6 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 6 years ago

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

22b6833 GPL-3

bioc-issue-bot commented 6 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.

hstern2 commented 6 years ago

the package is now giving OK for the builds/checks on linux and mac.

the PC build is giving a warning

Warning: file 'retnfit/cleanup' did not have execute permissions: corrected Warning: file 'retnfit/configure' did not have execute permissions: corrected

despite these being labeled as Executable File on github. I am not sure how to fix that (or if it needs to be fixed) - I found a thread on bioc-devel that indicates this should be ignored?

https://www.mail-archive.com/bioc-devel@r-project.org/msg06776.html

Harry

mtmorgan commented 6 years ago

@hstern2 Yes ignore the warning; I'll review your package when the VERSION BUMP REQUIRED tag is removed (by a version bump on your end). Thanks!

bioc-issue-bot commented 6 years ago

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

2e6dbe6 version bump

bioc-issue-bot commented 6 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.

hstern2 commented 6 years ago

OK I bumped version to 0.99.8

On Jan 11, 2018, at 10:56 AM, Martin Morgan notifications@github.com<mailto:notifications@github.com> wrote:

@hstern2https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_hstern2&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=v5CH1EB_hSEkiVi9IwfKj8XLwZMca1FRN-B5TMExuWI&s=P0pTOp2cFuF8mXGfYoDd-oX9f4dvRyu65cbe1Jj8X0Q&e= Yes ignore the warning; I'll review your package when the VERSION BUMP REQUIRED tag is removed (by a version bump on your end). Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Bioconductor_Contributions_issues_376-23issuecomment-2D356971176&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=v5CH1EB_hSEkiVi9IwfKj8XLwZMca1FRN-B5TMExuWI&s=UkWBvLcx387UeRxXc4LqFUUbw_6mwnr2OpMejY_0Q6Q&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ASdGsl0PO4g0ilYQSwf-5Fhq-5FvXc2-2Dr2T6ks5tJi88gaJpZM4Nj7yB&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=v5CH1EB_hSEkiVi9IwfKj8XLwZMca1FRN-B5TMExuWI&s=eVgypYWCi4l0ElESFYnZGZo-X_QgerBTr21PwtVOvEk&e=.

mtmorgan commented 6 years ago

General

do not commit 'build' products to git, e.g., remove build/
the configure file looks like (the unmaintainable) output from a much simpler configure.ac; provide configure.ac instead / as well
It's pretty unusual to code MPI at the C level, and much more flexible to make decisions like this at the R level via Rmpi, e.g., your package ability is determined by system capabilities at package installation, rather than at run time.
Make sure NEWS can be parsed by utils::news()

DESCRIPTION, NAMESPACE

Please provide additional detail in the Deescription: field, e.g., about inputs, and merits relative to alternative approaches.
Since

R

stop(paste(...)) can usually be replaced with stop(...)
More carefully validate user-suplied inputs, e.g., using stopifnot()

Use BiocParallel::SerialParam() (and Import: BiocParallel, etc) to avoid extra complexity in the main function, e.g.,

if (## MPI is available) {
   ...
    bp_param <- SnowParam(n_proc, type="MPI")
} else {
    bp_param <- SerialParam()
}

bplapply(..., BPPARAM = bp_param)

src

looks like R_logical() is just Rf_ScalarLogical(); likewise R_int() / Rf_ScalarInteger().
Usual convention is INTEGER(x)[0] rather than INTEGER_VALUE(), and INTEGER(x) rather than INTEGER_POINTER(x), etc. Also STRING_ELT(x, 0)
Is there sufficient care to avoid race conditions in the writes at, e.g., gn.c:653?

vignette / man

The vignette is very brief. Consider developing a more extensive example, including visualizing and interpretting the result.
The vignette and man page are too succinct in their description of experiment_set and other inputs. Provide enough description and 'worked example' to remove or explain jargon, e.g, whait are 'experiment' and 'index' nodes, what does an outcome -1, 0, 1 signify? How are 'costs' measured, in what (experimental?) context is a perturbation (and is this really logical TRUE / FALSE)?

bioc-issue-bot commented 6 years ago

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

08ad027 configure.ac

bioc-issue-bot commented 6 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.

hstern2 commented 6 years ago

hi Martin thanks for the great feedback. I am working on these changes now, I have one quick question, now that I have a configure.ac file I am getting a new warning for the tokay2 build

Found the following shell script(s) with CR or CRLF line endings: configure.ac

is there an easy fix, should I ignore (like the execute perms thing…??)

thanks - Harry

On Jan 18, 2018, at 4:12 PM, Martin Morgan notifications@github.com<mailto:notifications@github.com> wrote:

General

do not commit 'build' products to git, e.g., remove build/
the configure file looks like (the unmaintainable) output from a much simpler configure.ac; provide configure.ac instead / as well
It's pretty unusual to code MPI at the C level, and much more flexible to make decisions like this at the R level via Rmpi, e.g., your package ability is determined by system capabilities at package installation, rather than at run time.
Make sure NEWS can be parsed by utils::news()

DESCRIPTION, NAMESPACE

Please provide additional detail in the Deescription: field, e.g., about inputs, and merits relative to alternative approaches.
Since

R

stop(paste(...)) can usually be replaced with stop(...)
More carefully validate user-suplied inputs, e.g., using stopifnot()
Use BiocParallel::SerialParam() (and Import: BiocParallel, etc) to avoid extra complexity in the main function, e.g.,

if (## MPI is available) { ... bp_param <- SnowParam(n_proc, type="MPI") } else { bp_param <- SerialParam() }

bplapply(..., BPPARAM = bp_param)

src

looks like R_logical() is just Rf_ScalarLogical(); likewise R_int() / Rf_ScalarInteger().
Usual convention is INTEGER(x)[0] rather than INTEGER_VALUE(), and INTEGER(x) rather than INTEGER_POINTER(x), etc. Also STRING_ELT(x, 0)
Is there sufficient care to avoid race conditions in the writes at, e.g., gn.c:653?

vignette / man

The vignette is very brief. Consider developing a more extensive example, including visualizing and interpretting the result.
The vignette and man page are too succinct in their description of experiment_set and other inputs. Provide enough description and 'worked example' to remove or explain jargon, e.g, whait are 'experiment' and 'index' nodes, what does an outcome -1, 0, 1 signify? How are 'costs' measured, in what (experimental?) context is a perturbation (and is this really logical TRUE / FALSE)?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Bioconductor_Contributions_issues_376-23issuecomment-2D358783191&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=qK5--WhYhsPuVUYfGqyt9P0Fq1cqUeIGImVB-fnfP3U&s=USrYsCnrVtid8Kdc0MRY4I89I2SOrzd71TkjicgCf8k&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ASdGsobkPmZGwt8iRatpBJ9zRHA0EdO7ks5tL7PPgaJpZM4Nj7yB&d=DwMFaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=qK5--WhYhsPuVUYfGqyt9P0Fq1cqUeIGImVB-fnfP3U&s=JT-xQ-esBq48-RLYUiPUn7q6gmz2lV9jBCvUF3EU_Ug&e=.

mtmorgan commented 6 years ago

I think the solution is to save configure.ac with 'linux' (i.e., linefeed only) line endings; probably this (saving with LF line endings) is an option i whatever editor you are using.

bioc-issue-bot commented 6 years ago

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

8a18aac version bump

bioc-issue-bot commented 6 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 6 years ago

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

8713d90 version bump

bioc-issue-bot commented 6 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 6 years ago

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

959787d set core.autocrlf to input

bioc-issue-bot commented 6 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 6 years ago

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

f4420bf .gitattributes

bioc-issue-bot commented 6 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.

hstern2 commented 6 years ago

OK, the file was created on linux and didn’t have any CR’s but I think git was adding them when it checked out for the windows system. after looking around a bit and trying a couple things I ended up creating and checking in a .gitattributes file with the line

configure.ac eol=lf

that removed the warning.

On Jan 23, 2018, at 2:39 PM, Martin Morgan notifications@github.com<mailto:notifications@github.com> wrote:

I think the solution is to save configure.ac with 'linux' (i.e., linefeed only) line endings; probably this (saving with LF line endings) is an option i whatever editor you are using.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Bioconductor_Contributions_issues_376-23issuecomment-2D359905885&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=S95wwSoaCVpnumwbR7oAPYLCxdrPOgDuxKYAda3ymZI&s=D0NrK9hFv579b-UVbn4w1x2c5oD_a9d9QQcashCgnJs&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ASdGsn3L-5F86PHLEEnUk1FM0byjSC-5FqDwks5tNjWKgaJpZM4Nj7yB&d=DwMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=u5vYjhgzTwkkBS1kIx9w6AP8ISqhIMUy3rigf-BuXE4&m=S95wwSoaCVpnumwbR7oAPYLCxdrPOgDuxKYAda3ymZI&s=9fziVyXxT7vewtakaF4TIeNsHmx7HT05iu3eJxEihSE&e=.

bioc-issue-bot commented 6 years ago

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

996285c version bump

bioc-issue-bot commented 6 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 6 years ago

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

de5fc8e Rf_asInteger, etc c9bf6b5 version bump

bioc-issue-bot commented 6 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.

Bioconductor / Contributions

(inactive) retnfit #376

General

DESCRIPTION, NAMESPACE

R

src

vignette / man