phemd - Githubissues

wschen commented 6 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/wschen/phemd

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 Git (optionally via GitHub).

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

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: "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.

wschen commented 6 years ago

@LiNk-NY Thank you! I have updated the DESCRIPTION file and will update those couple of remaining minor edits soon.

LiNk-NY commented 6 years ago

Hi William, @wschen

It looks like you didn't apply the diff. You should add two spaces in the second line of the Description field. Please do that and bump the package version.

-Marcel

bioc-issue-bot commented 6 years ago

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

7d0e48d Fixed formatting issue

wschen commented 6 years ago

@LiNk-NY Sorry about that – should be correct now!

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.

LiNk-NY commented 6 years ago

Hi William, @wschen It doesn't look like you've removed the is(obj, "Phemd") calls. Please do so and don't forget to bump the version. Thanks, Marcel

bioc-issue-bot commented 6 years ago

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

baf785c Removed extraneous class checking

wschen commented 6 years ago

@LiNk-NY I've removed those calls from the method functions!

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: "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.

LiNk-NY commented 6 years ago

Hi William, @wschen Thanks for making those changes. Your package may be taking a bit too long to check. If possible, please simplify the examples. Also, please avoid if condition checking on a vector:

Found the following possibly unsafe calls:
Warning in if (e3 == "asNamespace") e3 <- as.character(e[[4L]][[2L]]) :
  the condition has length > 1 and only the first element will be used
Warning in if (e3 == "asNamespace") e3 <- as.character(e[[4L]][[2L]]) :
  the condition has length > 1 and only the first element will be used
File 'phemd/R/functions-plotting.R':
  assignInNamespace(x = "draw_colnames", value = "drawColnames45", 
    ns = asNamespace("pheatmap"))
  assignInNamespace(x = "draw_colnames", value = "drawColnames45", 
    ns = asNamespace("pheatmap"))

Use identical or all.equal.

Regards, Marcel

bioc-issue-bot commented 6 years ago

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

e744d03 shortened examples to avoid timeout

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.

wschen commented 6 years ago

@LiNk-NY Thanks for this suggestion. Since the functions in my package are all meant to be run sequentially, I could not shorten the examples but I have asked that R not check a few of the examples which involve (relatively simple) saving of plots or results. Note that these examples were checked successfully in a prior builds without problems or timeout on Linux and OS X and should work properly.

Regarding the "if" vector checking – that warning actually comes from the "asNamespace" function which is an R built-in. As a result, I cannot modify the code but I believe I am calling the function properly and it should not cause any problems despite the warning.

Best, Will

LiNk-NY commented 6 years ago

Hi William, @wschen

Please avoid using dontrun tags in examples. You can possibly shorten the run times by using smaller example datasets and reducing the number of plots.
Remove save* functionality from plots. Plots should always output to the graphic device and not to be stored in the user's file system. It should be up to the user to save a plot.

Regarding the "if" vector checking – that warning actually comes from the "asNamespace" function which is an R built-in. As a result, I cannot modify the code but I believe I am calling the function properly and it should not cause any problems despite the warning.

No, it means that length(e3) > 1L. You should avoid using logical vectors in if functions. To remedy this, replace e3 == "asNamespace" with all.equal(e3, "asNamespace") or (identical(...)). This will force e3 to be of length 1.

Regards, Marcel

wschen commented 6 years ago

@LiNk-NY I will work on the shorter examples and remove the save option from plotting functions.

Meanwhile, I am still unsure how to remedy the e3 == "asNamespace" call. From what I can tell, this 'e3 == "asNamespace"' check is performed by the R build / check process and is definitely not code that I wrote so I am unable to change it. A quick search online brought me to this source file (under the .check_package_code_tampers function):

http://mtweb.cs.ucl.ac.uk/mus/bin/install_R/R-3.1.1/build/src/library/tools/all.R

If using the "assignInNamespace" function is not allowed (which I am fairly confident is the ultimate culprit of this function call), I can remove this functionality from my package altogether (or open to other suggestions as to how to address this).

Thanks, Will

LiNk-NY commented 6 years ago

Hi Will, @wschen You're right. My apologies. I think the use of assignInNamespace is triggering that. Could you avoid using that function?

Also, you have a number of undefined global variables:

Undefined global functions or variables:
  assay assayData assays estimateDispersions estimateSizeFactors exprs
  featureData featureData<- pData phenoData seqlen seurat.combined
  varLabels<-

Please import the appropriate package(s) and functions such as Biobase.

wschen commented 6 years ago

@LiNk-NY No problem, glad we've figured that out. Unfortunately I don't know of a way around that function as it modifies one small part of a CRAN heatmap plotting function to make the heatmap marker labels much more readable. It is theoretically possible to exclude this altogether although I think the functionality as I've implemented it currently is beneficial for the user and downstream viewers.

I will look into importing the appropriate packages for those global variables as well.

LiNk-NY commented 5 years ago

Hi William, @wschen You could try contacting the maintainer and asking to let users provide hjust and rot parameters in the main function. This will avoid you from replacing the function in the package namespace. Best regards, Marcel

LiNk-NY commented 5 years ago

Hi William, @wschen Any updates on the status of the package? Did you contact the pheatmap maintainer? Regards, Marcel

wschen commented 5 years ago

@LiNk-NY Thanks for the suggestion. I have contacted the pheatmap maintainer and am awaiting a reply now. I am also still addressing the other changes you suggested and hope to resubmit an updated version soon.

LiNk-NY commented 5 years ago

Hi Wiliam, @wschen Could you provide further updates on the status of the phemd package? Best regards, Marcel

bioc-issue-bot commented 5 years ago

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

7df264c Updated plotting functions

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

bioc-issue-bot commented 5 years ago

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

5719f87 Version bump

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

bioc-issue-bot commented 5 years ago

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

fa5ddb0 Updated plotting and data files

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

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

ec1eda0 Fixed list indexing

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

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

342b392 Updated plotting function

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

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

744611f Updated R dependency to >= 3.6

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

bioc-issue-bot commented 5 years ago

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

3256ba5 Reverted to stable version of R (3.5.0)

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

wschen commented 5 years ago

@LiNk-NY I have addressed the issues we discussed as follows:

Removed "dontrun" tags from examples for all exported functions
Removed "save" functionality from all plotting functions as requested
Removed all calls to assignInNamespace – I have gotten an update from the pheatmap maintainer who has updated the CRAN package to allow for rotation of the column labels. My function allows for the additional parameter to be passed into the pheatmap plotting function.
Imported all packages so that there should no longer be any undefined global functions or variables.

A remaining automatic "warning" is due to a single "set.seed" call which is important so that users can generate reproducible results. Another "warning" is due to my package requiring R version >= 3.5.0 instead of >= 3.6.0 – but doing so causes the build to fail on tokay2 (so I kept the requirement as R version >=3.5.0 for now). I hope these changes are sufficient for my package to be accepted!

Thanks, Will

bioc-issue-bot commented 5 years ago

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

9f9d1c1 Updated vignettes

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

wschen commented 5 years ago

@LiNk-NY Just wanted to make sure you saw my latest message above the last couple of bot comments. Please let me know what you think when you get a chance.

Best, Will

LiNk-NY commented 5 years ago

Hi William, @wschen Thanks for following up.

I strongly recommend that you do not set any seeds inside functions. For users to ensure reproducibility, they should set the same seed outside the function when sharing code.
I still see a lot of dontrun tag in R/Phemd-methods.R and associated man pages. Please remove them and provide a runnable example so that you don't potentially get stale code.

Almost there! Thanks, Marcel

LiNk-NY commented 5 years ago

Hi William, @wschen Any updates on making the requested changes to the package? It is almost ready to be accepted.

Best regards, Marcel

bioc-issue-bot commented 5 years ago

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

52242f0 Updated examples and optional seed

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

wschen commented 5 years ago

@LiNk-NY I have updated the examples so that they should all be run now. I have also updated the function to allow for an optional seed to be passed in as a parameter and have documented it clearly in the man page. I think this is preferable for the specific use case I'm implementing – hope this is ok!

Will

LiNk-NY commented 5 years ago

Hi William, @wschen

Setting a default seed value is not the way to ensure reproducibility.

Users wishing to obtain reproducible results would create a script and set their own seed within the script and not inside the function.

Please remove the set.seed function from your code.

Regards, Marcel

CC: @mtmorgan

mtmorgan commented 5 years ago

The user expects that

set.seed(123)
your_function()
rnorm(1)

will produce different results than

set.seed(234)
your_function()
rnorm(1)

but if your_function() does set.seed(345) then their attempt is thwarted.

On the other hand if they'd like to implement reproducibility for your function, then they would be able to

set.seed(456)
your_function_without_set_seed()

So there is nothing to be gained by setting the seed inside your function, and much frustration to be introduced.

bioc-issue-bot commented 5 years ago

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

7dd6cc0 Removed set.seed call inside function

wschen commented 5 years ago

@LiNk-NY Got it, removed the set.seed call inside the function!

Will

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

phemd #897