Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sparseMatrixStats #1465

Closed
8 tasks done
const-ae opened this issue Apr 3, 2020 · 32 comments
Closed
8 tasks done

sparseMatrixStats #1465

const-ae opened this issue Apr 3, 2020 · 32 comments
Assignees
Labels
3a. accepted will be ingested into Bioconductor daily builder for distribution WARNINGS

Comments

@const-ae
Copy link

const-ae commented Apr 3, 2020

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 understand that by submitting my package to Bioconductor,
    the package source and all review commentary are visible to the
    general public.

  • I have read the Bioconductor Package Submission
    instructions. My package is consistent with the Bioconductor
    Package Guidelines.

  • 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.

  • My package addresses statistical or bioinformatic issues related
    to the analysis and comprehension of high throughput genomic data.

  • 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:

  • The 'devel' branch for new packages and features.
  • The stable 'release' branch, made available every six
    months, for bug fixes.
  • 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
Copy link
Collaborator

Hi @const-ae

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: sparseMatrixStats
Type: Package
Title: Summary Statistics for Rows and Columns of Sparse Matrices
Version: 0.99.0
Authors@R: person("Constantin", "Ahlmann-Eltze", email = "artjom31415@googlemail.com", 
        role = c("aut", "cre"), comment = c(ORCID = "0000-0002-3762-068X"))
Description: High performance functions for row and column operations on sparse matrices.
    For example: col / rowMeans2, col / rowMedians, col / rowVars etc. Currently,
    the optimizations are limited to data in the column sparse format. 
    This package is inspired by the matrixStats package by Henrik Bengtsson.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.0.2
LinkingTo: 
    Rcpp
Imports: 
    Rcpp,
    Matrix,
    MatrixGenerics,
    matrixStats,
    methods
Suggests: 
    testthat (>= 2.1.0),
    knitr,
    bench,
    rmarkdown,
    BiocStyle
SystemRequirements: C++14
VignetteBuilder: knitr
biocViews: Infrastructure, Software, DataRepresentation

@bioc-issue-bot
Copy link
Collaborator

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

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 bioc-issue-bot added 2. review in progress assign a reviewer and a more thorough review of package code and documentation taking place and removed 1. awaiting moderation labels Apr 3, 2020
@bioc-issue-bot
Copy link
Collaborator

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.

@lawremi
Copy link

lawremi commented Apr 3, 2020

Shouldn't this (and MatrixGenerics) go into CRAN?

@const-ae
Copy link
Author

const-ae commented Apr 3, 2020

I don't necessarily disagree ;)

I guess the case for sparseMatrixStats being on BioConductor is that MatrixGenerics was submitted here and it makes sense to have them together. More generally, a lot of functionality around S4 is here on BioConductor (like S4Vectors), so it felt appropriate for me to submit both packages here as well.
But you are right, that the packages could be of interest for the whole R community :)

@PeteHaitch and @hpages, what would you say about this?

@PeteHaitch
Copy link

Shouldn't this (and MatrixGenerics) go into CRAN?

I also don't necessarily disagree :)
There's been a fair bit of bike-shedding and going around in circles on this package and its extensions (including very much myself in this; see Bioconductor/MatrixGenerics#2 (comment)).
It's good to have an actual implementation of it - I hope this can help clarify where it should live.

@lawremi
Copy link

lawremi commented Apr 3, 2020

Thanks for introducing me to the term "bike-shedding" and the law of triviality.

@PeteHaitch
Copy link

@lawremi I didn't mean that to dismiss your suggestion or concerns, which are well-placed. This really reflects my frustrations with myself for not being able to nail down the implementation

@mtmorgan
Copy link
Contributor

mtmorgan commented Apr 6, 2020

@hpages has advocated for (initial) submission to Bioconductor, where it is much easier to mature packages, pointing to rhdf5 as an example

@lshep
Copy link
Contributor

lshep commented Apr 6, 2020

I'll do some behinds the scenes stuff too to have this be able to build without waiting for MartrixGenerics to be accepted and without submitting under the other issue. hand tight and I'll kick off a new build when I have it set up so I can move forward with the review

@lawremi
Copy link

lawremi commented Apr 6, 2020

I recognize that Bioconductor wins in terms of developer convenience, but our community should be more proactive about sharing our general solutions and distributing through CRAN. Otherwise, only we will benefit.

@bioc-issue-bot
Copy link
Collaborator

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.

@lshep
Copy link
Contributor

lshep commented Apr 6, 2020

It is our understanding that R doesn't support C++14 on Windows yet. Please update to C++11

@const-ae
Copy link
Author

const-ae commented Apr 7, 2020

Hi Lori,
thanks for kicking of another build and "do some behinds the scenes stuff". That's highly appreciated :)

It is our understanding that R doesn't support C++14 on Windows yet

Oh, that is unfortunate. I thought that R 4.0 also meant Rtools 40 and thus finally a modern compiler on windows.
If not, I will see that I how I can rewrite the internals to avoid the C++14 features.

@PeteHaitch
Copy link

I thought that R 4.0 also meant Rtools 40 and thus finally a modern compiler on windows.

It seems like its still being decided; see https://stat.ethz.ch/pipermail/r-devel/2020-April/079253.html

@lshep
Copy link
Contributor

lshep commented Apr 7, 2020

We are ready to go with the Rtools40 whenever CRAN decides to switch over but as far as we are aware it is still being discussed. @hpages Do you know if this is truly the case when we switch to the new toolchain?

@hpages
Copy link
Contributor

hpages commented Apr 7, 2020

Yep, we should be ready to switch (we have set up builds with the new toolchain for a small subset of packages).

As for whether or not R core is going to switch in time for the R 4.0.0 release, I don't know more about this than what's being currently discussed on the R-devel mailing list. The thing is that we can't switch before they switch, which means that for now the SPB still has to use the old toolchain. So no C++14 features yet.

So here you go: all the cards are on the table and I let you guys decide the strategy for getting sparseMatrixStats into Bioconductor in time for the release.

@lshep
Copy link
Contributor

lshep commented Apr 13, 2020

We can wait to see when/if R 4.0.0 gives the official word that they will use the new toolchain. Until then it is up to you if you decide to modify the code to work in the meantime or temporarily mark as unsupported on windows until the switch happens.

The package is generally well written and I only have the few minor comments:

General

  • are duplicate copies of the license and readme necessary?

README

  • Include Bioconductor installation instructions

vignette

  • If the set.seed is required for users to reproduce the output in the vignette then this should be shown.

I won't be able to accept this package until MatrixGenerics gets accepted first.

@bioc-issue-bot
Copy link
Collaborator

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

3c0ea53 Remove all C++14 feature I mainly used generic la...
a81ef6a Bump version

@bioc-issue-bot
Copy link
Collaborator

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
Copy link
Collaborator

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

cbf1b0a Bump version

@bioc-issue-bot
Copy link
Collaborator

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.

@const-ae
Copy link
Author

I have adapted the C++ code in a way that the C++14 features are not used anymore and it should now build on Windows with Rtools 35.

I have also added the Bioconductor installation instructions to the README and made sure the set.seed(1) is visible in the vignette. Thank you for the comments.

are duplicate copies of the license and readme necessary?

I have deleted the LICENSE.md file as it was not necessary. The two README files, I would say, are necessary, because the .Rmd file generates the .md file that is then displayed on GitHub.


However, the BiocCheck() is now failing for my package, but I don't understand why:

* Checking package NEWS...
Error in file.path(pkgdir, pkg, c("inst", "inst", "inst", ".", "."), c("NEWS.Rd",  : 
  object 'pkg' not found
Calls: <Anonymous> -> BiocCheck -> checkNEWS -> file.path
Execution halted

I don't remember that is was an issue before. Is it related to the fact that MatrixGenerics is not yet accepted?

@const-ae
Copy link
Author

Ah I see the NEWS error was already fixed with Bioconductor/BiocCheck@27ea5e2.
I will just wait and bump the version tomorrow to see if the fix has propagated until then :)

@lshep
Copy link
Contributor

lshep commented Apr 14, 2020

The NEWS file ERROR is on our end. We have a fix implemented in linux and there will be a fix on windows by later this afternoon. I will kick off a manual build so there is at least an accurate linux build. it should post shortly.

@bioc-issue-bot
Copy link
Collaborator

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.

@lshep
Copy link
Contributor

lshep commented Apr 15, 2020

Great as soon as MatrixGenerics is accepted I will accept this package. Cheers

@PeteHaitch
Copy link

PeteHaitch commented Apr 16, 2020

@const-ae I know you already changed the code, but it looks like the migration to rtools40 is happening: https://twitter.com/opencpu/status/1250452381403398149 and https://community-bioc.slack.com/archives/CEQ04GKEC/p1586999356141000

@const-ae
Copy link
Author

Hi, thanks for the links :)
In that case, I am wondering if I should just revert commit const-ae/sparseMatrixStats@3c0ea53.

@lori, the MatrixGenerics package has been accepted tonight (#1448 (comment)) :)

@lshep
Copy link
Contributor

lshep commented Apr 16, 2020

Yes we updated the R and tool chain on windows last night. Up to you if you want to revert code. accepting the package regardless.

@lshep lshep added 3a. accepted will be ingested into Bioconductor daily builder for distribution and removed 2. review in progress assign a reviewer and a more thorough review of package code and documentation taking place labels Apr 16, 2020
@bioc-issue-bot
Copy link
Collaborator

Your package has been accepted. It will be added to the
Bioconductor Git repository and nightly builds. Additional
information will be posed to this issue in the next several
days.

Thank you for contributing to Bioconductor!

@mtmorgan
Copy link
Contributor

The master branch of your GitHub repository has been added to Bioconductor's git repository.

To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/const-ae.keys is not empty), then no further steps are required. Otherwise, do the following:

  1. Add an SSH key to your github account
  2. Submit your SSH key to Bioconductor

See further instructions at

https://bioconductor.org/developers/how-to/git/

for working with this repository. See especially

https://bioconductor.org/developers/how-to/git/new-package-workflow/
https://bioconductor.org/developers/how-to/git/sync-existing-repositories/

to keep your GitHub and Bioconductor repositories in sync.

Your package will be included in the next nigthly 'devel' build (check-out from git at about 6 pm Eastern; build completion around 2pm Eastern the next day) at

https://bioconductor.org/checkResults/

(Builds sometimes fail, so ensure that the date stamps on the main landing page are consistent with the addition of your package). Once the package builds successfully, you package will be available for download in the 'Devel' version of Bioconductor using BiocManager::install("sparseMatrixStats"). The package 'landing page' will be created at

https://bioconductor.org/packages/sparseMatrixStats

If you have any questions, please contact the bioc-devel mailing list (https://stat.ethz.ch/mailman/listinfo/bioc-devel); this issue will not be monitored further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3a. accepted will be ingested into Bioconductor daily builder for distribution WARNINGS
Projects
None yet
Development

No branches or pull requests

7 participants