Open mpadge opened 3 years ago
alexpghayes
commented
3 years ago autotests results have been dealt with -- there are some other ongoing build failures related to vignettes and a convergence criterion. I believe things should be in a state where reviewers can give this a look, but I am also unable to replicate the convergence issues locally on my computer and reviewers might run into that immediately, just FYI.
Update: I've dealt with both of these issues.
mpadge
commented
3 years ago Thanks @alexpghayes. I still see some remaining autotest things, condensed here for brevity to what I deem as the relevant ones. (The remainder are primarily diagnostic messages about ranges for parameters like rank and max_iter; admissable ranges exist in practice, yet the documentation makes no indication of any ranges, so maybe adding some kind of helpful advice in the documentation on likely or expected ranges for these parameters would be useful.)
| type | fn_name | parameter | operation | content | yaml_hash | |
|---|---|---|---|---|---|---|
| 5 | diagnostic | adaptive_impute | rank | length 2 vector for single-length parameter | parameter [rank] is assumed to be a single value of integer type, yet admits vectors of length > 1 | c23cbd |
| 8 | diagnostic | adaptive_impute | max_iter | length 2 vector for single-length parameter | parameter [max_iter] is assumed to be a single value of integer type, yet admits vectors of length > 1 | c23cbd |
| 11 | diagnostic | adaptive_impute | check_interval | length 2 vector for single-length parameter | parameter [check_interval] is assumed to be a single value of integer type, yet admits vectors of length > 1 | c23cbd |
| 16 | diagnostic | adaptive_impute | rank | length 2 vector for single-length parameter | parameter [rank] is assumed to be a single value of integer type, yet admits vectors of length > 1 | 747b85 |
| 19 | diagnostic | adaptive_impute | max_iter | length 2 vector for single-length parameter | parameter [max_iter] is assumed to be a single value of integer type, yet admits vectors of length > 1 | 747b85 |
| 20 | diagnostic | adaptive_impute | initialization | lower case character parameter | Parameter initialization of function [adaptive_impute] is assumed to a single character, but is case dependent | 747b85 |
| 21 | diagnostic | adaptive_impute | initialization | upper case character parameter | Parameter initialization of function [adaptive_impute] is assumed to a single character, but is case dependent | 747b85 |
| 22 | diagnostic | adaptive_impute | initialization | length 2 vector for single-length parameter | parameter [initialization] is assumed to be a single value of character type, yet admits vectors of length > 1 | 747b85 |
| 25 | diagnostic | adaptive_impute | check_interval | length 2 vector for single-length parameter | parameter [check_interval] is assumed to be a single value of integer type, yet admits vectors of length > 1 | 747b85 |
| 32 | diagnostic | citation_impute | rank | length 2 vector for single-length parameter | parameter [rank] is assumed to be a single value of integer type, yet admits vectors of length > 1 | 2cd207 |
| 35 | diagnostic | citation_impute | max_iter | length 2 vector for single-length parameter | parameter [max_iter] is assumed to be a single value of integer type, yet admits vectors of length > 1 | 2cd207 |
| 38 | diagnostic | citation_impute | check_interval | length 2 vector for single-length parameter | parameter [check_interval] is assumed to be a single value of integer type, yet admits vectors of length > 1 | 2cd207 |
| 39 | diagnostic | citation_impute | initialization | lower case character parameter | Parameter initialization of function [citation_impute] is assumed to a single character, but is case dependent | 2cd207 |
| 40 | diagnostic | citation_impute | initialization | upper case character parameter | Parameter initialization of function [citation_impute] is assumed to a single character, but is case dependent | 2cd207 |
| 41 | diagnostic | citation_impute | initialization | length 2 vector for single-length parameter | parameter [initialization] is assumed to be a single value of character type, yet admits vectors of length > 1 | 2cd207 |
summary (x)
#> autotesting package [fastadi, v0.0.0.9011] generated 41 rows of output of the following types:
#> 0 errors
#> 3 warnings
#> 3 messages
#> 35 other diagnosticss
#> That corresponds to 13.667 messages per documented function (which has examples)
#>
#> fn_name num_errors num_warnings num_messages num_diagnostics
#> 1 adaptive_impute NA 2 2 21
#> 2 adaptive_initialize NA NA NA 2
#> 3 citation_impute NA 1 1 12
#>
#> git hash for package as analysed here:
#> [27e052cc42d6a7e618eb111e31ba97d14ae7d59c]Created on 2020-10-07 by the reprex package (v0.3.0)
alexpghayes
commented
3 years ago All of these are addressed in my commit from above, but perhaps not in ways that autotest is aware of?
mpadge
commented
3 years ago oh great, then that'll give me a chance to compare currently coded autotest expectations with your actual implementation. Will report back...
mpadge
commented
3 years ago Reporting back: It was a bug in autotest. Everything now checks clear except that the parameter initialization of citation_impute() is case-dependent. That's obviously very superficial, so up to you whether you want to address it or not. Next steps will appear here very soon. I'll also delete the autotest output from above, along with the previous couple of comments, if that's okay with you?
alexpghayes
commented
3 years ago That's a little weird since citation_impute() is essentially a copy-paste of adaptive_impute() with slightly different computations, but equivalent error checking. I'm happy to ignore the case matching, I think that's a pretty low priority given that I already use arg.match().
mpadge
commented
3 years ago Thanks @alexpghayes for your submission and improvements that have already been made to the package. Don't worry about the remaining autotest output. Most of it can be ignored in this case, and the few things that can't are ultimately going to be optional (like case-matching). The testing regime is currently hard-coded, but will soon allow user-specified regimes to be applied. We also plan to implement a system to record written explanations for any categories of tests you choose to disable, with your justifications ultimately incorporated within initial package submission/pre-review templates. None of that is yet possible, so we're happy to just move on, but will likely re-visit with autotest at a later stage, for which we have all the relevant git hash information.
Given that, we would now like to proceed to the formal review stage, for which members of the project's advisory board @bbolker and @topepo have kindly agreed to review your package. They are now requested to perform a two-stage review, the first part involving assessment of the package against the standards as they are currently drafted, with the second being a more "traditional" review. We hope, by the time we proceed to this second component, that many aspects which might otherwise have arisen within a "traditional" unstructured review will already have been covered, and will thereby make the review process notably easier.
Our review system will ultimately perform much of the preceding automated assessment prior to actual submission, and reviewers will be provided with a high-level interactive graphical overview of the package's functions and their inter-relationships. In lieu of the system being that far, reviewers can clone Alex's repo from github.com/RoheLab/fastadi, then run the following three lines in the brolgar directory:
remotes::install_github("mpadge/packgraph")
library(packgraph)
pg_graph(".")That should give you an interactive version something like this:
@bbolker and @topepo, could you please now asses the fastadi package with respect to the current General Standards for Statistical Software, and the category-specific standards for Unsupervised Learning Software.
Please do this in two phases:
In each case, please only note those standards which you judge the package not to conform to, along with a description of what you would expect this particular software package to do in order to conform to each standard. When you do that, please provide sufficient information on which standard you are referring to. (The standards themselves are all enumerated, but not yet at a necessarily stable state, so please provide enough information for anyone to clearly know which standard you are referring to regardless of potential changes in nomenclature.) Please also note as a separate list all those standards which you think should not apply to this package, along with brief explanations of why.
Importantly, to aid us in refining the standards which will ultimately guide the peer review of statistical software, we also ask you to please consider whether you perceive any aspects of software (design, functionality, algorithmic implementations or applications, testing, and any other aspects you can think of) which you think might be able to be addressed by standards, and yet which are not addressed by our standards in their current form.
In particular, we note that the nominated category "Dimensionality Reduction, Clustering, and Unsupervised Learning" only partially describes @alexpghayes's package, notably because our categories effectively aim to describe the general aims of software, whereas in this case that category applies to much of the methodology, while the actual aim remains arguably beyond that scope. We will therefore be particularly interested in hearing your thoughts on the applicability or otherwise of the category-specific standards in this case.
To sum up, please post the following in this issue:
Once you've done that, we'll ask to you proceed to a more general review of the software, for which we'll provide more detail at that time. Thanks all for agreeing to be part of this!
We would like to have this review phase completed within 4 weeks, so by the 13th of November 2020. We accordingly suggest that you aim to have the first of the two tasks completed within two weeks, by the 30th October.
Could you both please also record approximately how much time you have spent on each review stage. Thank you!
mpadge
commented
3 years ago Update for reviewers @bbolker and @topepo, note that this repo now includes an R package which enables you to get a pre-formatted checklist for your reviews (inspired by, and with gratitude to, co-board member @stephaniehicks) by running the following lines:
remotes::install_github("ropenscilabs/statistical-software-review")
library(statsoftrev) # the name of the package
rssr_standards_checklist (category = "unsupervised")That will produce a markdown-formatted checklist in your clipboard ready to paste where you like, or you can use a filename parameter to specify a local file.
ping @noamross so you'll be notified of these conversations.
bbolker
commented
3 years ago OK, will do. Note that I had to use githubinstall::githubinstall("pkgapi") (this package searches github for relevant repos rather than requiring you to know the repo owner in advance) in order to install pkgapi: remotes::install_github() doesn't know how to get arbitrary dependencies from GitHub, only from CRAN.
bbolker
commented
3 years ago also, there's a typo "funtions" in the packgraph messages shown in the first comment above (sorry)
bbolker
commented
3 years ago In vignettes/README/DESCRIPTION only; could be more prominent?
Not sure about this. Uses standard definitions (the bulk of this package is mathematical and algorithmic rather than statistical)
roxygen2 to document all functions.
roxygen2 format, along with a final @noRd tag to suppress automatic generation of .Rd files. no associated pubs [yet? vignette looks like a ms in early stages of prep: readme says " the vignettes are currently scratch work for reference by the developers and are not yet ready for general consumption."
[README says " In simulations fastadi often outperforms softImpute by a small margin.", but I don't know if/where this code lives]
adaptive_impute(...,max_iter=20.5) runs without error; also adaptive_impute(..., additional=c(20,25))
match.arg() or equivalent where applicable to only permit expected values.tolower() or equivalent to ensure input of character parameters is not case dependent; or explicitly document that parameters are strictly case-sensitive.integer via as.integer()as.numeric()as.character() (and not paste or paste0)as.factor()as...() functionsfactor type, secondary documentation should explicitly state whether these should be ordered or not, and those inputs should provide appropriate error or other routines to ensure inputs follow these expectations. sf-format data) or added (such as insertion of variable or column names where none were provided). Software only accepts sparse matrices.
NA) data, with options minimally including:
na.rm = FALSE-type parameters (such as mean(), sd() or cor()).NaN, Inf and -Inf), including potentially ignoring or removing such values. Inf or NA values give TridiagEigen: eigen decomposition failed. This is an imputation method, but I don't actually understand enough about the area to know how missing values are filled? I guess missing values are coded as structural zeros???
stop(), warning(), message(), or equivalent should be unique
multiple "This should not happen", "max_iter must be an integer >= 1L" errors, "Reached maximum allowed iterations" warnings?NA) or undefined (NaN, Inf) values, the absence of any such values in return objects should be explicitly tested. NA fields or columns or all identical fields or columns.Machine$double.eps) to data does not meaningfully change results<MYPKG>_EXTENDED_TESTS=1 environment variable.CONTRIBUTING.md or tests/README.md file.vector or matrix form, or that all inputs must be in data.frame form with both column and row names.attributes(), to corresponding aspects of return objects.
scale() or equivalent transformations without explaining why scale is applied, and explicitly illustrating and contrasting the consequences of not applying such transformations. center and scale. parameters of the stats::prcomp() function).factor, and should provide ways to explicitly avoid any default transformations (with error or warning conditions where appropriate).NA or NaN values from Inf values (for example, through use of na.omit() and related functions from the stats package).Documentation points out that results will be unreliable if a rank is chosen for approximation that is approximately equal to, or greater than, the rank of the input. (Testing the rank of a large matrix is expensive and probably impractical for typical input data; don't know if it is worth pointing out Matrix::rankMatrix() or not
array-like data with no row names) should provide an additional parameter to enable cases to be labelled. print method which provides an on-screen summary of model (input) parameters and methods used to generate results. The print method may also summarise statistical aspects of the output data or results.
print method should always ensure only a restricted number of rows of any result matrices or equivalent are printed to the screen. summary methods for model objects which should summarise the primary statistics used in generating the model (such as numbers of observations, parameters of methods applied). The summary method may also provide summary statistics from the resultant model. plot methods, either through explicit implementation, extension of methods for existing model objects, through ensuring default methods work appropriately, or through explicit reference to helper packages such as factoextra and associated functions.plot method is NOT a generic plot method dispatched on the class of return objects (that is, through a plot.<myclass> function), that method dispatch should nevertheless exist in order to explicitly direct users to the appropriate function.bbolker
commented
3 years ago sparseMatrix zoo of classes that will cause problems?? Presumably these will throw TridiagEigen: eigen decomposition failed errors ...?fastadi, the README file (not included with the package), or the vignettes (which are not installed: "the vignettes are currently scratch work for reference by the developers and are not yet ready for general consumption.").remotes::install_github()alexpghayes
commented
3 years ago This feedback is very useful, thank you!
Thanks @alexpghayes for volunteering to submit your
fastadipackage for trial "soft submission" to rOpenSci's new system for peer review of statistical software. This issue includes output from our automated assement and reporting tools developed as part of this new system. These currently function as the following two distinct components (which will be integrated later):packgraphautotestCreated on 2020-10-06 by the reprex package (v0.3.0)
Most of these diagnostic messages are about the admissable ranges of single-value parameters, and can probably be safely ignored, or maybe taken to indicate a minor need to tweak documentation of the parameters to indicate expected or admitted ranges. The parsing of description entries by
autotestto estimate stated ranges is currently fairly crude, so updates of documentation on your side to address these could provide useful test cases for refinement of those procedures.Those aside, the only issues are generally lack of control for lengths of parameters presumed to be single-valued, and matching character arguments regardless of case. If you could please ping here once you've addressed those, we'll post an updated
autotestoutput and proceed to subsequent stages of the review process. Thanks!Further information on autotest output
The output of
autotestincludes a columnyaml_hash. This in turn refers to the yaml specification used to generate the autotests, which can be generated locally by runningexamples_to_yaml (<path>/<to>/<package>). Those contain theyaml_hashvalues, and finding the matching value will show you the base code used to trigger the diagnostic messages. The operation column should then provide a sufficient description of what has been mutated with regard to the structure defined in the yaml.