search

ropensci / software-review

rOpenSci Software Peer Review.
286 stars 104 forks source link

dendroNetwork #627

Closed RonaldVisser closed 2 months ago

RonaldVisser commented 4 months ago

Date accepted: 2024-04-25

Submitting Author Name: Ronald M. Visser Submitting Author Github Handle: !--author1-->@RonaldVisser<!--end-author1-- Repository: https://github.com/RonaldVisser/dendroNetwork Version submitted: 0.5.0 Submission type: Standard Editor: !--editor-->@ldecicco-USGS<!--end-editor-- Reviewers: @kaijagahm, @gzach93

Archive: TBD Version accepted: TBD Language: en

Package: dendroNetwork
Type: Package
Title: Create networks of dendrochronological series using pairwise similarity
Version: 0.5.0
Authors@R: 
    c(person(given ="Ronald", 
            family = "Visser", 
            email = "r.m.visser@saxion.nl", 
            role = c("aut", "cre"),
            comment = c(ORCID = "0000-0001-6966-1729")),
    person(given = "Angelino", 
            family ="Salentino",
            role = "ctb",
            comment = c(ORCID = "0000-0002-4763-3943")),
   person(given = "Andy", 
            family ="Bunn",
            role = "ctb",
            comment = c(ORCID = "0000-0001-9027-2162")))
Maintainer: Ronald Visser <r.m.visser@saxion.nl>
Depends: R (>= 3.5.0)
Imports: 
    dplR (>= 1.7.2),
    igraph,
    stringr,
    reshape2,
    RCy3,
    dplyr,
    RColorBrewer,
    tidyr,
    foreach,
    magrittr,
    doParallel,
    lifecycle, 
    stats,
    grDevices
Description: This package makes it easy to create dendrochronological networks based on the similarity between tree-ring series or chronologies. It includes various functions to compare tree-ring curves building upon dplR. The networks can be used to visualise and understand the relations between tree-ring curves. These networks are also very useful to estimate the provenance or wood-use within a structure/context/site.
License: GPL (>= 3)
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.3.1
Suggests: 
    testthat (>= 3.0.0)
Config/testthat/edition: 3
Roxygen: list(markdown = TRUE)

Scope

I tried to run pkgcheck, but it failed with the following feedback:

Error in vapply(tag_data, function(i) i$keyword, character(1L)) : values must be length 1, but FUN(X[[12]]) result is length 2

I have absolutely no idea why....

The same error occurs locally and using github action.

Technical checks

Confirm each of the following by checking the box.

This package:

Publication options

MEE Options - [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)

Code of conduct

ropensci-review-bot commented 4 months ago

Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type @ropensci-review-bot help for help.

ropensci-review-bot commented 4 months ago

:rocket:

Editor check started

:wave:

ropensci-review-bot commented 4 months ago

Note: The following R packages were unable to be installed/upgraded on our system: [RCy3]; some checks may be unreliable.

ldecicco-USGS commented 4 months ago

I'm guessing the issue is that RCy3 is only on Bioconductor. The mentions I found in the rOpenSci guide:

https://devguide.ropensci.org/building.html?q=Biocond#pkgdependencies

And: "If you develop a package depending on or intended for Bioconductor, you might find biocthis relevant."

I'll poke around more and see if I can find other recent submissions that used Bioconductor packages.

mpadge commented 4 months ago

Sorry @ldecicco-USGS and @RonaldVisser, that was a bug with Bioc installs on our side. Checks should appear in a moment.

ropensci-review-bot commented 4 months ago

Checks for [dendroNetwork (v0.5.0)]()

git hash: 02a3fe68

Important: All failing checks above must be addressed prior to proceeding

(Checks marked with :eyes: may be optionally addressed.)

Package License: GPL (>= 3)

1. Package Dependencies

Details 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. |type |package | ncalls| |:----------|:-------------|------:| |internal |base | 164| |internal |dendroNetwork | 20| |imports |igraph | 25| |imports |magrittr | 20| |imports |reshape2 | 10| |imports |RCy3 | 9| |imports |dplyr | 7| |imports |stats | 6| |imports |grDevices | 4| |imports |RColorBrewer | 3| |imports |dplR | 2| |imports |foreach | 2| |imports |doParallel | 1| |imports |stringr | NA| |imports |tidyr | NA| |imports |lifecycle | NA| |suggests |knitr | NA| |suggests |rmarkdown | NA| |suggests |testthat | NA| |linking_to |NA | NA| 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()', and examining the 'external_calls' table.

base

length (20), lapply (10), subset (10), unlist (10), max (9), for (8), min (8), as.vector (6), c (6), matrix (5), ncol (5), apply (4), as.numeric (4), rownames (4), by (3), cbind (3), colSums (3), dim (3), formatC (3), merge (3), nchar (3), nrow (3), paste0 (3), sqrt (3), system.file (3), unique (3), diff (2), is.na (2), mean (2), rep (2), seq (2), seq_along (2), abs (1), all.vars (1), as.data.frame (1), list (1), log (1), match.call (1), sum (1)

igraph

cliques (6), add_edges (4), make_empty_graph (4), V (3), E (2), graph.data.frame (2), clique_num (1), cluster_edge_betweenness (1), decompose (1), decompose.graph (1)

dendroNetwork

cor_mat_overlap (3), t_value (3), wuchswerte (3), clique_community_names_par (2), sim_table (2), clique_community_names (1), cyto_clean_styles (1), cyto_create_cpm_style (1), cyto_create_gn_style (1), cyto_create_graph (1), dendro_network (1), find_all_cpm_com (1)

magrittr

%>% (20)

reshape2

melt (10)

RCy3

createNetworkFromIgraph (4), importVisualStyles (3), layoutNetwork (1), loadTableData (1)

dplyr

mutate (5), row_number (2)

stats

D (3), weights (2), pnorm (1)

grDevices

colorRampPalette (2), colors (2)

RColorBrewer

brewer.pal (3)

dplR

as.rwl (2)

foreach

foreach (2)

doParallel

registerDoParallel (1)

**NOTE:** Some imported packages appear to have no associated function calls; please ensure with author that these 'Imports' are listed appropriately.

2. Statistical Properties

This 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: - code in R (100% in 16 files) and - 1 authors - 1 vignette - 2 internal data files - 14 imported packages - 13 exported functions (median 29 lines of code) - 13 non-exported functions in R (median 28 lines of code) --- Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages The following terminology is used: - `loc` = "Lines of Code" - `fn` = "function" - `exp`/`not_exp` = exported / not exported All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by [the `checks_to_markdown()` function](https://docs.ropensci.org/pkgcheck/reference/checks_to_markdown.html) The final measure (`fn_call_network_size`) is the total number of calls between functions (in R), or more abstract relationships between code objects in other languages. Values are flagged as "noteworthy" when they lie in the upper or lower 5th percentile. |measure | value| percentile|noteworthy | |:------------------------|------:|----------:|:----------| |files_R | 16| 74.9| | |files_vignettes | 3| 92.4| | |files_tests | 3| 75.2| | |loc_R | 434| 43.1| | |loc_vignettes | 161| 41.5| | |loc_tests | 12| 9.0| | |num_vignettes | 1| 64.8| | |data_size_total | 112963| 84.1| | |data_size_median | 56481| 88.8| | |n_fns_r | 26| 35.7| | |n_fns_r_exported | 13| 53.9| | |n_fns_r_not_exported | 13| 28.8| | |n_fns_per_file_r | 1| 0.2|TRUE | |num_params_per_fn | 3| 33.6| | |loc_per_fn_r | 28| 74.9| | |loc_per_fn_r_exp | 29| 61.6| | |loc_per_fn_r_not_exp | 28| 76.0| | |rel_whitespace_R | 5| 19.2| | |rel_whitespace_vignettes | 21| 25.8| | |rel_whitespace_tests | 17| 5.2| | |doclines_per_fn_exp | 31| 34.8| | |doclines_per_fn_not_exp | 0| 0.0|TRUE | |fn_call_network_size | 7| 27.1| | ---

2a. Network visualisation

Click to see the interactive network visualisation of calls between objects in package

3. goodpractice and other checks

Details of goodpractice checks (click to open)

--- #### 3b. `goodpractice` results #### `R CMD check` with [rcmdcheck](https://r-lib.github.io/rcmdcheck/) R CMD check generated the following error: 1. Error in proc$get_built_file() : Build process failed R CMD check generated the following check_fails: 1. description_url 2. description_bugreports #### Test coverage with [covr](https://covr.r-lib.org/) ERROR: Test Coverage Failed #### Cyclocomplexity with [cyclocomp](https://github.com/MangoTheCat/cyclocomp) Error : Build failed, unknown error, standard output: * checking for file ‘dendroNetwork/DESCRIPTION’ ... OK * preparing ‘dendroNetwork’: * checking DESCRIPTION meta-information ... OK * installing the package to build vignettes * creating vignettes ... ERROR --- re-building ‘dendroNetwork_use.Rmd’ using rmarkdown Quitting from lines 21-44 [flowchart_workflow] (dendroNetwork_use.Rmd) Error: processing vignette 'dendroNetwork_use.Rmd' failed with diagnostics: there is no package called 'DiagrammeR' --- failed re-building ‘dendroNetwork_use.Rmd’ SUMMARY: processing the following file failed: ‘dendroNetwork_use.Rmd’ Error: Vignette re-building failed. Execution halted  #### Static code analyses with [lintr](https://github.com/jimhester/lintr) [lintr](https://github.com/jimhester/lintr) found the following 89 potential issues: message | number of times --- | --- Avoid 1:length(...) expressions, use seq_len. | 2 Avoid library() and require() calls in packages | 2 Lines should not be more than 80 characters. | 85

4. Other Checks

Details of other checks (click to open)

:heavy_multiplication_x: The following function name is duplicated in other packages: - - `sim_table` from BCEA

Package Versions

|package |version | |:--------|:--------| |pkgstats |0.1.3.11 | |pkgcheck |0.1.2.15 |

Editor-in-Chief Instructions:

Processing may not proceed until the items marked with :heavy_multiplication_x: have been resolved.

ldecicco-USGS commented 4 months ago

Thanks for the submission @RonaldVisser ! Looks like a neat package.

Before we can proceed you'll need to get continuous integration set up. I'd start by trying: https://usethis.r-lib.org/reference/github_actions.html#use-github-action-check-standard-

That might work - I haven't used Bioconductor packages in awhile, so I'm not 100% sure. If that doesn't work, then you could try: https://rdrr.io/bioc/biocthis/man/use_bioc_github_action.html

The other "X" items above should be pretty straightforward to address, but let me know if you have any questions.

The error on the build is:

Quitting from lines 21-44 [flowchart_workflow] (dendroNetwork_use.Rmd)
Error: processing vignette 'dendroNetwork_use.Rmd' failed with diagnostics:
there is no package called 'DiagrammeR'
--- failed re-building ‘dendroNetwork_use.Rmd’

If DiagrammeR isn't used in any of the package functions, you'll want to add it to "Suggests" for that vignette to be generated.

RonaldVisser commented 4 months ago

Thanks for the compliment! Thank you for the comments and help!

I have done some updating of various functions, added some tests and the continuous integration (github actions R-CMD / pkgcheck) seems to be working fine. According to pkgcheck it now checks all the boxes :)

Due to the various edits, I released a new release (0.5.1), with changes described in the NEWS-file.

ldecicco-USGS commented 4 months ago

@ropensci-review-bot check package

ropensci-review-bot commented 4 months ago

Thanks, about to send the query.

ropensci-review-bot commented 4 months ago

:rocket:

Editor check started

:wave:

mpadge commented 4 months ago

Apologies once again @ldecicco-USGS @RonaldVisser. Failed this time because the checks appear to have been called at exactly the time the system was scheduled for regular rebuild (first few hours of Tues, UTC). I've just submitted a PR to our DevGuide to clarify that. Checks will appear straight after this.

ropensci-review-bot commented 4 months ago

Checks for dendroNetwork (v0.5.2)

git hash: e76e3dd1

(Checks marked with :eyes: may be optionally addressed.)

Package License: GPL (>= 3)

1. Package Dependencies

Details 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. |type |package | ncalls| |:----------|:-------------|------:| |internal |base | 164| |internal |dendroNetwork | 20| |imports |igraph | 25| |imports |magrittr | 20| |imports |reshape2 | 10| |imports |RCy3 | 9| |imports |dplyr | 7| |imports |stats | 6| |imports |grDevices | 4| |imports |RColorBrewer | 3| |imports |dplR | 2| |imports |foreach | 2| |imports |doParallel | 1| |imports |stringr | NA| |imports |tidyr | NA| |imports |lifecycle | NA| |suggests |knitr | NA| |suggests |rmarkdown | NA| |suggests |testthat | NA| |suggests |DiagrammeR | NA| |linking_to |NA | NA| 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()', and examining the 'external_calls' table.

base

length (20), lapply (10), subset (10), unlist (10), max (9), for (8), min (8), as.vector (6), c (6), matrix (5), ncol (5), apply (4), as.numeric (4), rownames (4), by (3), cbind (3), colSums (3), dim (3), formatC (3), merge (3), nchar (3), nrow (3), paste0 (3), sqrt (3), system.file (3), unique (3), diff (2), is.na (2), mean (2), rep (2), seq (2), seq_along (2), abs (1), all.vars (1), as.data.frame (1), list (1), log (1), match.call (1), sum (1)

igraph

cliques (6), add_edges (4), make_empty_graph (4), V (3), decompose (2), E (2), graph_from_data_frame (2), clique_num (1), cluster_edge_betweenness (1)

dendroNetwork

cor_mat_overlap (3), t_value (3), wuchswerte (3), clique_community_names_par (2), sim_table (2), clique_community_names (1), cyto_clean_styles (1), cyto_create_cpm_style (1), cyto_create_gn_style (1), cyto_create_graph (1), dendro_network (1), find_all_cpm_com (1)

magrittr

%>% (20)

reshape2

melt (10)

RCy3

createNetworkFromIgraph (4), importVisualStyles (3), layoutNetwork (1), loadTableData (1)

dplyr

mutate (5), row_number (2)

stats

D (3), weights (2), pnorm (1)

grDevices

colorRampPalette (2), colors (2)

RColorBrewer

brewer.pal (3)

dplR

as.rwl (2)

foreach

foreach (2)

doParallel

registerDoParallel (1)

**NOTE:** Some imported packages appear to have no associated function calls; please ensure with author that these 'Imports' are listed appropriately.

2. Statistical Properties

This 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: - code in R (100% in 16 files) and - 1 authors - 1 vignette - 2 internal data files - 14 imported packages - 13 exported functions (median 29 lines of code) - 13 non-exported functions in R (median 28 lines of code) --- Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages The following terminology is used: - `loc` = "Lines of Code" - `fn` = "function" - `exp`/`not_exp` = exported / not exported All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by [the `checks_to_markdown()` function](https://docs.ropensci.org/pkgcheck/reference/checks_to_markdown.html) The final measure (`fn_call_network_size`) is the total number of calls between functions (in R), or more abstract relationships between code objects in other languages. Values are flagged as "noteworthy" when they lie in the upper or lower 5th percentile. |measure | value| percentile|noteworthy | |:------------------------|------:|----------:|:----------| |files_R | 16| 74.9| | |files_vignettes | 3| 92.4| | |files_tests | 8| 88.2| | |loc_R | 434| 43.1| | |loc_vignettes | 161| 41.5| | |loc_tests | 30| 17.7| | |num_vignettes | 1| 64.8| | |data_size_total | 112963| 84.1| | |data_size_median | 56481| 88.8| | |n_fns_r | 26| 35.7| | |n_fns_r_exported | 13| 53.9| | |n_fns_r_not_exported | 13| 28.8| | |n_fns_per_file_r | 1| 0.2|TRUE | |num_params_per_fn | 3| 33.6| | |loc_per_fn_r | 28| 74.9| | |loc_per_fn_r_exp | 29| 61.6| | |loc_per_fn_r_not_exp | 28| 76.0| | |rel_whitespace_R | 5| 19.2| | |rel_whitespace_vignettes | 21| 25.8| | |rel_whitespace_tests | 7| 5.2| | |doclines_per_fn_exp | 32| 36.7| | |doclines_per_fn_not_exp | 0| 0.0|TRUE | |fn_call_network_size | 7| 27.1| | ---

2a. Network visualisation

Click to see the interactive network visualisation of calls between objects in package

3. goodpractice and other checks

Details of goodpractice checks (click to open)

#### 3a. Continuous Integration Badges [![pkgcheck.yaml](https://github.com/RonaldVisser/dendroNetwork/actions/workflows/pkgcheck.yaml/badge.svg)](https://github.com/RonaldVisser/dendroNetwork/actions) [![R-CMD-check.yaml](https://github.com/RonaldVisser/dendroNetwork/actions/workflows/R-CMD-check.yaml/badge.svg)](https://github.com/RonaldVisser/dendroNetwork/actions) **GitHub Workflow Results** | id|name |conclusion |sha | run_number|date | |----------:|:-----------|:----------|:------|----------:|:----------| | 7901669279|pkgcheck |success |e76e3d | 22|2024-02-14 | | 7901669272|R-CMD-check |success |e76e3d | 10|2024-02-14 | --- #### 3b. `goodpractice` results #### `R CMD check` with [rcmdcheck](https://r-lib.github.io/rcmdcheck/) R CMD check generated the following notes: 1. checking for hidden files and directories ... NOTE Found the following hidden files and directories: .github These were most likely included in error. See section ‘Package structure’ in the ‘Writing R Extensions’ manual. 2. checking top-level files ... NOTE File LICENSE is not mentioned in the DESCRIPTION file. 3. checking package subdirectories ... NOTE Found the following CITATION file in a non-standard place: CITATION.cff Most likely ‘inst/CITATION’ should be used instead. 4. checking R code for possible problems ... NOTE clique_community_names: no visible binding for global variable ‘com’ clique_community_names: no visible binding for global variable ‘com_name’ clique_community_names_par: no visible binding for global variable ‘i’ clique_community_names_par: no visible binding for global variable ‘com’ clique_community_names_par: no visible binding for global variable ‘com_name’ dendro_network: no visible binding for global variable ‘r’ dendro_network: no visible binding for global variable ‘sgc’ dendro_network: no visible binding for global variable ‘p’ dendro_network: no visible binding for global variable ‘series_a’ dendro_network: no visible binding for global variable ‘series_b’ find_all_cpm_com: no visible binding for global variable ‘node’ find_all_cpm_com: no visible binding for global variable ‘com_name’ find_all_cpm_com: no visible binding for global variable ‘n’ gn_names: no visible binding for global variable ‘GN_com’ gn_names: no visible binding for global variable ‘node’ gn_names: no visible binding for global variable ‘com_name’ Undefined global functions or variables: com com_name GN_com i n node p r series_a series_b sgc R CMD check generated the following check_fails: 1. rcmdcheck_stale_license_file 2. rcmdcheck_citation_file_at_standard_place 3. rcmdcheck_undefined_globals 4. rcmdcheck_hidden_files_and_directories #### Test coverage with [covr](https://covr.r-lib.org/) Package coverage: 91.44 #### Cyclocomplexity with [cyclocomp](https://github.com/MangoTheCat/cyclocomp) The following function have cyclocomplexity >= 15: function | cyclocomplexity --- | --- cyto_clean_styles | 20 #### Static code analyses with [lintr](https://github.com/jimhester/lintr) [lintr](https://github.com/jimhester/lintr) found the following 96 potential issues: message | number of times --- | --- Avoid 1:length(...) expressions, use seq_len. | 2 Avoid library() and require() calls in packages | 2 Lines should not be more than 80 characters. | 92

4. Other Checks

Details of other checks (click to open)

:heavy_multiplication_x: The following function name is duplicated in other packages: - - `sim_table` from BCEA

Package Versions

|package |version | |:--------|:--------| |pkgstats |0.1.3.11 | |pkgcheck |0.1.2.15 |

Editor-in-Chief Instructions:

This package is in top shape and may be passed on to a handling editor

ldecicco-USGS commented 4 months ago

@ropensci-review-bot assign @ldecicco-USGS as editor

ropensci-review-bot commented 4 months ago

Assigned! @ldecicco-USGS is now the editor

ldecicco-USGS commented 4 months ago

Editor checks:

Editor comments

I was able to run the example functions from the readme and vignette. It would very helpful to push a compiled pkgdown site to a GitHub Page. You can set that up with:

usethis::use_pkgdown_github_pages()

Setting up a pkgdown site should take care of most of the "Documentation " subtasks. The other sub-category that could be improved is "not just perfunctory" vignettes. Perhaps you could take the contents of the "paper.md" and put that same information in a vignette?

I would also ask that you take a look at the following packages and make sure there's not specific overlap:

https://github.com/AndyBunn/dplR https://github.com/hanecakr/fellingdateR https://github.com/ropensci/MtreeRing

There's a good chance I'd be asking authors from those packages to do a review. I do not know enough about tree ring science to understand where the overlap might fall.

RonaldVisser commented 4 months ago

Thank you for your helpful comments! More steps to make my package better :)

The paper is there to submit to JOSS in a later stage, but I have added an extra Vignette and I'll think about developing more Vignettes.

I have created a pkgdown website: https://ronaldvisser.github.io/dendroNetwork/ and that was and interesting and fun thing to do :)

There is no overlap with the packages you mentioned:

I believe that the dendroNetwork package can work very well with these packages, but it adds new functionality to the available R-packages and I have tried to explain that in the paper (for JOSS) I have added in the repository.
Oh, for your information, there are more packages in R for dendrochronology and I have compiled a list in the past: https://ronaldvisser.github.io/Dendro_R/

ldecicco-USGS commented 4 months ago

@ropensci-review-bot assign @kaijagahm as reviewer

ropensci-review-bot commented 4 months ago

@kaijagahm added to the reviewers list. Review due date is 2024-03-14. Thanks @kaijagahm 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.

ropensci-review-bot commented 4 months ago

@kaijagahm: If you haven't done so, please fill this form for us to update our reviewers records.

ldecicco-USGS commented 3 months ago

@ropensci-review-bot assign @gzach93 as reviewer

ropensci-review-bot commented 3 months ago

@gzach93 added to the reviewers list. Review due date is 2024-03-28. Thanks @gzach93 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.

ropensci-review-bot commented 3 months ago

@gzach93: If you haven't done so, please fill this form for us to update our reviewers records.

ropensci-review-bot commented 3 months ago

:calendar: @kaijagahm you have 2 days left before the due date for your review (2024-03-14).

kaijagahm commented 3 months ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

Example in wuchswerte fails because object anos1 is not found.

library(dendroNetwork)
data(hol_rom)
dplR::as.rwl(apply(anos1, 2, wuchswerte))
#> Error in eval(expr, envir, enclos): object 'anos1' not found

Created on 2024-03-12 with reprex v2.1.0

t_value doesn’t return anything to console–consider including an explicit return statement.

library(dendroNetwork)
t_value(0.5, 100) # no output
a <- t_value(0.5, 100)
a
#> [1] 5.744563

Created on 2024-03-12 with reprex v2.1.0

cyto_create_gn_style(g_hol, gn_coms = g_hol_gn) fails when run as part of the example for cyto_create_gn_style. Seems to be unable to load a file?

library(dendroNetwork)
data(hol_rom)
sim_table_hol <- sim_table(hol_rom)
g_hol <- dendro_network(sim_table_hol)
hol_com_cpm_k3 <- clique_community_names(g_hol, k = 3)
cyto_create_cpm_style(g_hol, k = 3, com_k = hol_com_cpm_k3)
#> Error: Don't know how to read file:/private/var/folders/xy/lqbp515s27q8v1drkll9gynw0000gr/T/RtmppMCfZy/reprex-172671e49826-large-unau/

Created on 2024-03-12 with reprex v2.1.0

cyto_create_cpm_style fails similarly.

library(dendroNetwork)
data(hol_rom)
sim_table_hol <- sim_table(hol_rom)
g_hol <- dendro_network(sim_table_hol)
hol_com_cpm_k3 <- clique_community_names(g_hol, k = 3)
cyto_create_cpm_style(g_hol, k = 3, com_k = hol_com_cpm_k3)
#> Error: Don't know how to read file:/private/var/folders/xy/lqbp515s27q8v1drkll9gynw0000gr/T/RtmppMCfZy/reprex-172625011c73-alive-lark/

Created on 2024-03-12 with reprex v2.1.0

Missing contribution guidelines. Includes URL, BugReports, and Maintainer.

Functionality

I see that there are tests set up, but I am not familiar with how to run the tests for someone else’s package on my own local machine. If anyone can provide guidance on this, I’d be happy to take a look! I have run tests for my own package before, but never for a package I’ve installed.

Title: good

codemetar: I’m not familiar with this.

Platforms: runs on Mac; I can’t comment on other systems.

Package API: function names are in snake_case and are reasonably logical. Could be nice to preface them all with a common thing like dendro_ and make some of them shorter, but effort has already been made to make them short and consistent. I appreciate that the cytoscape-interface functions are all named with cyto_at the beginning, for instance.

Input checking: There is some input checking, and the error messages returned seem sensible. I would suggest doing a little more input checking, for example making sure that the network objects being passed to the functions are actually igraph network objects as expected. This goes hand in hand with my recommendation that you be a bit more specific about what data format is expected from users of the package. Keep in mind data types etc. and think about which ones will or won’t break your functions. I like to use the checkmate package for fast and easy argument checking, but there are many options including just more base R checks.

CITATION file: good

README etc: generally, multiple points of entry are done well. There is redundancy in where the information is presented, and I was mostly able to find out about the package and its core workflow through the README as well as the vignettes.

Documentation website: ?

Consider @family tag for function grouping

Estimated hours spent reviewing: 4

Review Comments

  1. I am coming to this review without any prior understanding of dendrochronology. Therefore, it would be helpful if you could provide more information about the data types. For example, ?hol_rom tells me that this dataset contains dendrochronological site chronologies. I don’t actually know what a “site chronology” is. What is an object of class “rwl”, and how would I go about making sure that an object I’m feeding into this function has class “rwl” (or is it okay if it’s just a data frame)? What does a single row in this dataset represent? What does a single column represent? What do the column names mean? It’s possible that anyone who would be using this package would already understand these things, but I think there’s no harm in being more explicit. When I go to use a package, typically my first question is “If I wanted to use this package on my data, what would my data have to look like, exactly, in order for the functions to work?”

  2. Regarding the documentation for sim_table: I think I would understand this better if I understood the the data better, as described in 1). It seems like you’re saying that trs1 (hol_rom, in this example) represents a single tree ring series (or is it multiple series? I can’t tell). trs2 is optional. I did some experimenting and found that if you don’t specify trs2, the function ends up comparing trs1 to itself; that is, identical(sim_table(hol_rom), sim_table(hol_rom, hol_rom)) is TRUE. In the description for the trs1 argument, maybe specify that behavior, instead of saying “to be compared with trs2.

  3. Carrying on with the theme of me being naive to the domain: I don’t understand what last_digit_radius does. What is a “series name” and what is the “radius of the tree-ring series”? What would be the units of the radius? Why does this argument matter? Again, if the answers to some of these questions would be obvious to domain-familiar users of your package, then maybe it’s not necessary to define them, but I was wondering.

  4. Make sure to specify in multiple places that Cytoscape has to be not only installed but actually running when you use the functions that relate to it.

  5. It would be nice if there was an example of how to use the package on large datasets. I recognize, though, that including a large dataset is probably not practical. Maybe use a small dataset as an example but still run it in parallel, using tic() and toc() to demonstrate the time difference? In the given example, it wasn’t clear that the object network in the example code wasn’t a real object and that the code wouldn’t actually run.

  6. cyto_create_cpm_style maybe didn’t work for me? I’m not exactly sure what was supposed to happen, but I didn’t see any colors or styling appearing, just black and white. I can provide more details/screenshots if that would help.

Some more detailed comments on the pre-cytoscape functions (some of this is maybe overly nitpicky, but it was my thought process as I was working through the functions!)

library(dendroNetwork) # load the package
data(hol_rom) # what is this? I have no knowledge of tree networks, so I don't understand what I'm looking at.
?hol_rom # pulls up some documentation. As someone with know domain knowledge, I don't understand what a "site chronology" is. What does a single row represent? What does each column represent?
sim_table_hol <- sim_table(hol_rom) # what format does the data need to be in for this function to work? Is this a standard format that everyone in the field will know? Are you sure? Can you be more explicit? What is a Rwl object?
# What does it mean to only use one dataset, versus to use two datasets compared to each other?
# I don't understand what `last_digit_radius` really does. What is a "series name" and what is the "radius of the tree-ring series"? What would the radius be measured in?
# I recommend explicitly defining each of the columns output
identical(sim_table(hol_rom), sim_table(hol_rom, hol_rom)) # these are identical. Seems that the sim_table function just compares trs1 to itself if trs2 isn't provided.

g_hol <- dendro_network(sim_table_hol)
g_hol_gn <- gn_names(g_hol) # in doc: "iGraph" should be "igraph", and consider linking to the function explicitly.
# in doc: for "Value", should say "A data frame. Column 1 contains node names; column 2 contains community names" or similar.
# Say explicitly what class of object needs to be passed to the function (g)
gn_names(hol_rom) # "Error in `ensure_igraph()`: ! Must provide a graph object (provided wrong object type). Run `rlang::last_trace()` to see where the error occurred." This is a good, informative error! Put it in the documentation to make it even clearer.
g_hol_cpm <- clique_community_names(g_hol, k = 2) # says "the default is set to smallest possible size (3) but if I put in k = 2 there is no error thrown.
# in Value, give an example of a community name, like "CPM_K2_4"
clique_community_names(g_hol, k = 3) # only gives some of the nodes, not all of them. Why?
clique_community_names(g_hol, k = 4) #"Error in clique_community_names(g_hol, k = 4) : The maximum clique size in the network is 3. Therefore k cannot exceed this number". But the documentation says that k is the *minimum*, not the max. What gives?
# Is this function supposed to be returning community assignments (in which case k is the number of communities to be found) or is it supposed to be returning communities with a minimum size of k?

hol_com_cpm_all <- find_all_cpm_com(g_hol) # I don't understand what is happening here
# seems to return in wide format. Why? Is this just the wide-format version of the previous function?
RonaldVisser commented 3 months ago

Hi @kaijagahm, thank you for the review and your detailed comments. These are very helpful for improving the package and the documentation, which makes me happy. I am looking forward to address the various things you pointed out and I hope to do this as soon as possible.

kaijagahm commented 3 months ago

You're very welcome! It was fun to explore this. I know my comments are a bit all over the place; I hope they are navigable. Feel free to ask if you encounter any confusion.

Except for some concerns over the examples running and one of the functions not showing colors in Cytoscape, and some documentation tweaks, most of my comments were around the margins and the package seems to be doing pretty well at its core functionality. I look forward to seeing how this develops!

ldecicco-USGS commented 3 months ago

@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/627#issuecomment-1993235710 time 4

ropensci-review-bot commented 3 months ago

Logged review for kaijagahm (hours: 4)

ropensci-review-bot commented 3 months ago

:calendar: @gzach93 you have 2 days left before the due date for your review (2024-03-28).

RonaldVisser commented 3 months ago

Response review Kaija Gahm

Thank you Kaija for the wonderfully detailed review. Below you will find the response to the review by kaijagahm.

Documentation

Functionality

ℹ Testing dendroNetwork
✔ | F W  S  OK | Context 
✔ |          1 | clique_community_names 
✔ |          1 | clique_community_names_par                                       
✔ |          2 | dendro_network                                                 
✔ |          2 | find_all_cpm_com                                                 
✔ |          2 | gn_names                                                         
✔ |          2 | sim_table                                                        
✔ |          1 | wuchwerte                                                        
══ Results ════════════════════════════════════════════════════════════════════════
Duration: 6.2 s 
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 11 ]

Review Comments

  1. Thanks for pointing this out. The packaged is aimed at dendrochronologists who would generally know what an rwl object is, or what a sitechronology is. I have added some explanation of the intended audience and guiding users to more information.

  2. I have added some more explanation to the help of the function sim_table() . I thought explained this sufficiently, but after reading your comments you have shown me that I was wrong. I really hope that it is more clear now.

  3. I think that this should be obvious to people in the domain. The naming of each series in a rwl data.frame generally follows a certain pattern (with deviations): Location_id, sample number and measurement (radius) number. ABC0011 would be location ABC, sample 001 and you can take several measurements along various radii in a tree trunk, in this case 1. You can make networks of measurements, but you can also group several radii into one node in the network. I hope that I am clear and that this explains it sufficiently?

  4. Thanks! I have checked and gone through everything and I hope that I have made this clear.

  5. Good point! I have created a separate vignette for this and explained that people should always use clique_community_names() initially and clique_community_names_par() only when the former is too slow. I have also update the readme to make this more clear.

  6. Oh thank you! Somewhere or sometime while writing the function I seem to have made a mistake. It is now working properly again!

The following commit addresses the points in 1-6.

I am really happy with all the things you have found and this resulted a new version to release: 0.5.2

gzach93 commented 2 months ago

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Documentation

The package includes all the following forms of documentation:

I think the author did a great job connecting their R package to the already existing R packages specifically created for their field.

The installation instructions were easy to follow and I was able to install the package and realted software needed.

It seems like the audience for this package would be new researchers to dendrology that are looking to create dendrochonological networks. If this is the intended audience maybe the author can add a little more about what Cytoscape is and does or provide a small summary and where the reader can find more resources? I want to make sure that I understand but is Cytoscape mainly used for the visualization after this R packages formats the data and run the network analyses?

Could you add the data description for the example data in the vignettes? I see it in RING_Visser_2021.Rd, but it might be good to move that into the vignette.

Can more specific details about what the code is doing be added? Consider also adding a brief description of things like the Girvan-Newman algorithm or more technical terms. Again I am assuming that this package is trying to create an easy pathway for researchers getting into this sort of analysis.

The t_value function could use a return function at the end of it. The example of the t_value function does not output results right now.

The wuchswerte function example, I think, require a dataset "anos1" from the dplR package. However, in order to use this dataset users have to visit https://opendendro.github.io/dplR-workshop/ to get started. Is it possible to make this data more accessible or use another dataset?

Functionality

I am not sure how to run these tests.

Estimated hours spent reviewing: 4

Review Comments

1) From my read through, I assumed that the audience was a new dendrologist of someone less familiar with creating these analyzes and this package creates an easier enterance point to help them. It think that this package does pretty good with this. However, maybe the vingettes can be expanded a little, offering another example dataset to work with and added definition for more field related jargon.

2) In the vignettes can you add a description of the example data. It might help new users better connect their datasets to the example data and think about the correct formatting needed.

3) I am not a dendrologist and I am not familiar with Cytoscape, so I am not sure if this is a familiar enough program where users do not need more information about what this program is used for or not. But, I think it might be useful to add more information about what Cytoscape is and some of the uses it has.

ldecicco-USGS commented 2 months ago

@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/627#issuecomment-2037644081 time 4

ropensci-review-bot commented 2 months ago

Logged review for gzach93 (hours: 4)

ldecicco-USGS commented 2 months ago

Thanks @gzach93! One way to run the tests (these are the "testthat" tests in the "test" folder) is to run devtools::test(). There are a few other options (you could go into the test folder and run them line-by-line even). The comment "Unit tests cover essential functions of the package and a reasonable range of inputs and conditions." means you are looking at the set of tests that the author includes to make sure they are testing a good-enough set of conditions on their functions. From the automated ropensci-bot, we know "Package coverage is 91.4%.". That's pretty good! It means 91.4% of the code is somehow tested in the testthat folder. The reviewer's job is just to make sure those tests seem adequate (which is subjective, but you can generally get a pretty good idea if the author's doing their due-diligence). If you knew of some important edge-cases for example, you would maybe want the developers to be testing those conditions.

@RonaldVisser you can now start addressing the reviews!

RonaldVisser commented 2 months ago

Response to review Zachary Gajewski (gzach93)

Thank you for your review, @gzach93. I am happy that you were positive about many things, but also for finding things to improve the package! Some things were already corrected before receiving your review and I am sorry for already updating while waiting for your review.

Documentation

Installation instructions: Thanks for noticing that. I have added a description of Cytoscape to the README and the vignettes: dendroNetwork and Cytoscape, including a link to the Cytoscape-website., see commit.

Vignettes:

I have added a vignette on dendrochronological data, see commit.

Links to a better explanation of the Girvan-Newman Algorithm and CPM were already added earlier, based on the review by @kaijagahm, see response and commit

Examples:

Thanks for also noticing that t_value() was not outputting to console. This was already corrected, based on the other review: see commit.

The references to the anos1 dataset was also already corrected, see commit

 Automated tests: see response to the review by kaijagahm

You were added as reviewer to the DESCRIPTION-file, see commit

Review Comments

  1. Thanks for pointing this out. I have update various things in the readme and the vignettes. I hope that this is now more clear
  2. I have added a vignette on the data, so this should help.
  3. I have added some description for Cytoscape and also some links to find more help.

I would like to explain that a dendrology is not the same as dendrochronology. Both are tree related, but the latter is aimed at tree-rings and the temporal patterns and the former more on trees in general. This confusion is very common, since these words look very similar.

Thank you again for your help in making this package and the documentation better!

RonaldVisser commented 2 months ago

@ldecicco-USGS : I have addressed both reviews.

I am really happy with the improvements based on the valuable comments by reviewers @kaijagahm and @gzach93, and I hope I have addressed the points they raised sufficiently.

ldecicco-USGS commented 2 months ago

Hi @gzach93 and @kaijagahm

Do you feel that the changes and updates have addressed your concerns?

If so, see: https://devdevguide.netlify.app/approval2template

ropensci-review-bot commented 2 months ago

@RonaldVisser: please post your response with @ropensci-review-bot submit response <url to issue comment> if you haven't done so already (this is an automatic reminder).

Here's the author guide for response. https://devguide.ropensci.org/authors-guide.html

gzach93 commented 2 months ago

Hi @RonaldVisser thank you for going through and addressing all my comments!

Reviewer Response #### Final approval (post-review) - [X] The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

ldecicco-USGS commented 2 months ago

Thanks again @gzach93 and @kaijagahm for the reviews and @RonaldVisser for the submission!

ldecicco-USGS commented 2 months ago

@ropensci-review-bot approve dendroNetwork

ropensci-review-bot commented 2 months ago

Approved! Thanks @RonaldVisser for submitting and @kaijagahm, @gzach93 for your reviews! :grin:

To-dos:

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent).

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.

kaijagahm commented 2 months ago

Reviewer Response

Final approval (post-review)

Estimated hours spent reviewing: 4.5 (total, not just for this step)

RonaldVisser commented 2 months ago

@ropensci-review-bot finalize transfer of dendroNetwork

ropensci-review-bot commented 2 months ago

Transfer completed. The dendroNetwork team is now owner of the repository and the author has been invited to the team

RonaldVisser commented 2 months ago

Thanks @ldecicco-USGS for your excellent role as editor! And thanks again @kaijagahm and @gzach93 for your wonderful reviews.

The package is now transferred to ROpenSci and I have addressed all to do's

I'd be happy to write a blog post about this packages @ropensci/blog-editors

steffilazerte commented 2 months ago

Hi @RonaldVisser!

I'm excited to hear that you're interested in writing a blog post for us!

As mentioned above, we have two types of blog posts: either an introduction to your package featuring technical details for a technical audience or a longer post with some narrative about its development or something you learned, and possibly an example of its use for a broader readership. The difference between a technote and a blog post is more about the style and content then length, but technical posts are often shorter than blog posts (no requirement either way).

To contribute a post, we'll have you open a PR to the roweb3 repository. About a week or two before our publication date, you can ping me and I'll review the post and give you some suggestions for content and style. You'll consider my feedback, make any changes you'd like to make, and then we merge and publish! We have a flexible timeline right now, so when every you're ready is good for us. However, if you work better with a deadline, we could set a deadline of the last week of May for the draft, expecting to publish the first week of June (or earlier!).

You can read more about the details of writing an rOpenSci blog post here: https://blogguide.ropensci.org/ (and don't hesitate to ask if you have any questions).

So! Let me know what kind of post you'd like to write and what kind of timeline you want to follow. You can reach me on the Slack (Steffi LaZerte; you should have gotten an invitation by now?) or via email (sel@steffilazerte.ca), and/or ping me when you open the PR to roweb3 (@steffilazerte).

Thanks!

RonaldVisser commented 1 month ago

Hi @steffilazerte ,

Thanks for the comments! I'll have a more through look at the documentation and I'll probably email you with questions later. I have not received a Slack-invite, but I have not used that for some time...

Thank you :)

steffilazerte commented 1 month ago

Great! Well you should get the Slack invite at some point (I think both people who handle that are away at the moment). And if you don't (and would like it 😁 ), let us know!