-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
fastMatMR
: Fast Matrix Market I/O
#606
Comments
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type |
🚀 Editor check started 👋 |
Also, not part of the issue template, but the package passes |
Checks for fastMatMR (v1.0.0.0)git hash: a910f6be
Important: All failing checks above must be addressed prior to proceeding Package License: MIT + file LICENSE 1. Package DependenciesDetails of Package Dependency Usage (click to open)
The table below tallies all function calls to all packages ('ncalls'), both internal (r-base + recommended, along with the package itself), and external (imported and suggested packages). 'NA' values indicate packages to which no identified calls to R functions could be found. Note that these results are generated by an automated code-tagging system which may not be entirely accurate.
Click below for tallies of functions used in each package. Locations of each call within this package may be generated locally by running 's <- pkgstats::pkgstats(<path/to/repo>)', and examining the 'external_calls' table. fastMatMRfmm_to_mat (1), fmm_to_vec (1), mat_to_fmm (1), release_questions (1), sparse_to_fmm (1), vec_to_fmm (1) NOTE: No imported packages appear to have associated function calls; please ensure with author that these 'Imports' are listed appropriately. 2. Statistical PropertiesThis package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing. Details of statistical properties (click to open)
The package has:
Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages
All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by the The final measure (
2a. Network visualisationClick to see the interactive network visualisation of calls between objects in package 3.
|
id | name | conclusion | sha | run_number | date |
---|---|---|---|---|---|
6056018810 | pages build and deployment | success | c3523b | 8 | 2023-09-02 |
6056006940 | pkgcheck | success | fe999d | 21 | 2023-09-02 |
6056006937 | pkgdown | success | fe999d | 23 | 2023-09-02 |
6056006936 | pre-commit | success | fe999d | 49 | 2023-09-02 |
6056006938 | R-CMD-check | success | fe999d | 39 | 2023-09-02 |
3b. goodpractice
results
R CMD check
with rcmdcheck
R CMD check generated the following note:
- checking installed package size ... NOTE
installed size is 11.1Mb
sub-directories of 1Mb or more:
libs 10.2Mb
R CMD check generated the following check_fail:
- rcmdcheck_reasonable_installed_size
Test coverage with covr
Package coverage: 68.1
The following files are not completely covered by tests:
file | coverage |
---|---|
inst/include/fast_matrix_market/chunking.hpp | 63.83% |
inst/include/fast_matrix_market/fast_matrix_market.hpp | 18.92% |
inst/include/fast_matrix_market/header.hpp | 59.69% |
inst/include/fast_matrix_market/read_body_threads.hpp | 67.44% |
inst/include/fast_matrix_market/read_body.hpp | 42.29% |
inst/include/fast_matrix_market/write_body.hpp | 50% |
R/misc.R | 0% |
Cyclocomplexity with cyclocomp
No functions have cyclocomplexity >= 15
Static code analyses with lintr
lintr found the following 1 potential issues:
message | number of times |
---|---|
Avoid library() and require() calls in packages | 1 |
Package Versions
package | version |
---|---|
pkgstats | 0.1.3.8 |
pkgcheck | 0.1.2.2 |
Editor-in-Chief Instructions:
Processing may not proceed until the items marked with ✖️ have been resolved.
FWIW there's > covr::package_coverage()
fastMatMR Coverage: 94.69%
src/from_file.cpp: 93.88%
src/to_file.cpp: 94.23%
R/fastMatMR-package.R: 100.00% |
@HaoZeke is indeed right there - the failing coverage check is a bug on our side, and not this submission. |
@ropensci-review-bot assign @maelle as editor |
Assigned! @maelle is now the editor |
👋 @HaoZeke! Nice to see you as an author this time around, thank you for your submission! 😀 A few comments/questions before we proceed to the reviewer search:
Thank you! |
^_^
This is because there is no need to expose bindings of the inner workings of the The user-facing C++ functions are in
That's a very cool library, I am currently using towncrier and tbump which are fairly generic tools.
Yup these are great suggestions, updated the package with them.
These are more version 2 issues and shouldn't block the review I think. It would take a while for me to get to them, the current functionality covers R matrices, vectors and sparse matrices from
You're welcome :) |
To clarify, the question was about tests specifically. Part of the code in inst/ is covered by tests anyway, so why not include them (and remove the unused code)? We do not have strict requirements on testing coverage for bundled code yet, but we can improve our guidance based on cases like this submission, so this discussion is important. |
Mostly maintainence. I have actually stripped the (already small) library of header files which are not used at all (Eigen / Armadillo / other C++ library compatibility files). In general, for bundled libraries it is a good idea to not tamper with the upstream source as much as possible (it makes it easier to update to new versions).
Yup, I understand. Policy wise I'd be even stricter, and say that bundled libraries must not be touched at all (other than whole-file removals if they are not relevant and self contained). This is standard practice in bundling libraries in C++ too, where projects might have different linting rules and It also allows people to write bindings as long as they understand the public API of the code. Often times users who want bindings are not fully aware / have the expertise to make decisions about code removal from (possibly) complicated upstream C++ code. |
Thank you for your thorough answer! I'll now look for reviewers. |
@ropensci-review-bot seeking reviewers |
Please add this badge to the README of your package repository: [![Status at rOpenSci Software Peer Review](https://badges.ropensci.org/606_status.svg)](https://github.com/ropensci/software-review/issues/606) Furthermore, if your package does not have a NEWS.md file yet, please create one to capture the changes made during the review process. See https://devguide.ropensci.org/releasing.html#news |
@ropensci-review-bot add @osorensen to reviewers |
@osorensen added to the reviewers list. Review due date is 2023-10-06. Thanks @osorensen for accepting to review! Please refer to our reviewer guide. rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more. |
@osorensen: If you haven't done so, please fill this form for us to update our reviewers records. |
@ropensci-review-bot add @czeildi to reviewers |
@czeildi added to the reviewers list. Review due date is 2023-10-07. Thanks @czeildi for accepting to review! Please refer to our reviewer guide. rOpenSci’s community is our best asset. We aim for reviews to be open, non-adversarial, and focused on improving software quality. Be respectful and kind! See our reviewers guide and code of conduct for more. |
@HaoZeke and @maelle, here is my initial review. My comments at the bottom explain a bit more about the boxes I haven't checked. Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Estimated hours spent reviewing: 2 hours
Review commentsDense matrices with the matrix packageIn README, you state that the
The command below lists all dense matrix types supported by the
Below is an example of a dense matrix from the
Statement of needThe README file contains a section titled “Why?”, but it does not state explicitly what problems the software is designed to solved nor what the target audience is. VignettesThere are vignettes, but they are very brief. Please provide some more extended examples, with more text explaining what is being done. ExamplesI understand that the |
Wonderful, thanks so much ^_^
Merging soon :)
I thought about this, but decided not to mainly because downstream package users (including me for other dependent packages) would want to call these functions where it makes sense (e.g. if my downstream package will only write matrix data or sparse matrix data) and CRAN disallows / warns calling unexported functions via |
Makes sense, thank you for the response! |
Thanks @HaoZeke, I have completed my checklist. |
@maelle since the reviews are satisfactorily completed (thanks all) I was wondering whta the next steps are :) |
👋 @HaoZeke! Thanks for your work! Thanks @czeildi @osorensen! Could please use the reviewer approval template for approval? Thanks! |
Reviewer ResponseFinal approval (post-review)
Estimated hours spent reviewing: 3 |
Reviewer ResponseFinal approval (post-review)
Estimated hours spent reviewing: 6 |
@ropensci-review-bot approve fastMatMR |
Approved! Thanks @HaoZeke for submitting and @osorensen, @czeildi for your reviews! 😁 To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @ropensci/blog-editors in your reply. They will get in touch about timing and can answer any questions. We maintain an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding (with advice on releases, package marketing, GitHub grooming); the guide also feature CRAN gotchas. Please tell us what could be improved. Last but not least, you can volunteer as a reviewer via filling a short form. |
@ropensci-review-bot submit review #606 (comment) time 3 |
Logged review for osorensen (hours: 3) |
@ropensci-review-bot submit review #606 (comment) time 6 |
Logged review for czeildi (hours: 6) |
@ropensci-review-bot finalize transfer of fastMatMR |
Transfer completed. |
Sorry to bring this up here, but, I can't seem to get the centralized documentation and r-universe builds to run, @maelle would you have any suggestions?
P.S. Thanks all for the help, this was a really useful process :) |
Also perhaps @mpadge would be better suited to offer advice on #606 (comment)? |
Also @ropensci/blog-editors, I think a blog post showing the enhanced inter-operability offered by this package might be of interest, if so, I'd be happy to discuss submitting a blog post :) |
@HaoZeke r-universe only builds at most every few hours, so my advice would be just wait and build should appear |
https://docs.ropensci.org/fastMatMR/ is up 🎉 The dev version of the dev guide has a package maintainer cheatsheet: https://devdevguide.netlify.app/maintenance_cheatsheet Thanks so much @HaoZeke for your participation in the process! |
Now on CRAN! |
wow that was fast, congrats!! |
Date accepted: 2023-10-30
Submitting Author Name: Rohit Goswami
Submitting Author Github Handle: @HaoZeke
Repository: https://github.com/HaoZeke/fastMatMR
Version submitted:
Submission type: Standard
Editor: @maelle
Reviewers: @osorensen, @czeildi
Archive: TBD
Version accepted: TBD
Language: en
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
The matrix market exchange formats are crucial to much of the ecosystem. The fastMatMR package focuses on high-performance read and write operations for Matrix Market files, serving as a key tool for data extraction in computational and data science pipelines.
Data scientists who might want to load and test the NIST matrices which include:
Additionally, this makes its simpler to interface to
scipy
and the rest of the data science ecosystem. This also includes working with the Tensor Algebra Compiler (TACO).The
Matrix
package inR
can perform similar operations but only for sparse matrices. ThefastMatMR
package not only provides enhanced performance but also extends support to dense matrices and vectors inbase R
, thus offering a more versatile solution.We have both read and write performance vignettes backing up the claims made.
(If applicable) Does your package comply with our guidance around Ethics, Data Privacy and Human Subjects Research?
If you made a pre-submission inquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Explain reasons for any
pkgcheck
items which your package is unable to pass.The package passes
pkcheck
: ropensci/fastMatMR#18, though the review bot disagrees :)Technical checks
Confirm each of the following by checking the box.
This package:
Publication options
Do you intend for this package to go on CRAN?
Do you intend for this package to go on Bioconductor?
Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:
MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: