fireexposuR: Compute and Visualize Wildfire Exposure #659
Comments
|
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type |
|
🚀 Editor check started 👋 |
Checks for fireexposuR (v1.0.0)git hash: aacfb397
(Checks marked with 👀 may be optionally addressed.) Package License: GPL (>= 3) 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. basec (27), exp (19), sum (11), cbind (9), data.frame (9), as.data.frame (8), paste (6), ifelse (5), by (4), levels (4), matrix (4), ncol (4), all (3), factor (3), labels (3), rep (3), seq (3), class (2), match (2), names (2), round (2), unique (2), as.factor (1), for (1), if (1), length (1), max (1), mean (1), missing (1), outer (1), plot (1), rbind (1), scale (1), table (1) magrittr%>% (95) dplyrmutate (43), count (4), case_when (1), summarise (1) ggplot2element_blank (9), ggplot (7), aes (6), element_text (5), coord_sf (4), geom_col (3), labs (3), element_rect (2), expansion (2), scale_fill_manual (2), geom_bar (1), geom_hline (1), geom_vline (1), scale_x_discrete (1), scale_y_continuous (1), theme (1) terraclassify (7), crop (6), focal (4), mask (4), project (4), vect (4), extract (3), res (3), rescale (3), spatSample (2), as.polygons (1), buffer (1), crs (1), merge (1), perim (1) tidyterrageom_spatraster_rgb (6), geom_spatvector (3), rename (2), filter (1), geom_spatraster (1), scale_fill_whitebox_c (1), whitebox.colors (1) fireexposuRexposure (7), direxp (2), adjustexp (1), extractexp (1), mapexpclass (1), mapexpcont (1), multidirexp (1) gridunit (12) statswindow (6), df (4) geospheredestPoint (8) maptilesget_credit (3), get_tiles (3) utilsdata (6) ggspatialannotation_scale (4) graphicstitle (4) tidyrpivot_wider (2), pivot_longer (1) grDevicespalette (2) MultiscaleDTMannulus_window (2) NOTE: Some imported packages appear to have no 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 |
|---|---|---|---|---|---|
| 11037129203 | pkgcheck | success | aacfb3 | 7 | 2024-09-25 |
| 11037129172 | R-CMD-check.yaml | success | aacfb3 | 7 | 2024-09-25 |
3b. goodpractice results
R CMD check with rcmdcheck
rcmdcheck found no errors, warnings, or notes
Test coverage with covr
Package coverage: 93.87
Cyclocomplexity with cyclocomp
The following function have cyclocomplexity >= 15:
| function | cyclocomplexity |
|---|---|
| extractexp | 17 |
Static code analyses with lintr
lintr found the following 10 potential issues:
| message | number of times |
|---|---|
| Avoid 1:length(...) expressions, use seq_len. | 1 |
| Avoid library() and require() calls in packages | 3 |
| Lines should not be more than 80 characters. This line is 83 characters. | 4 |
| Lines should not be more than 80 characters. This line is 84 characters. | 1 |
| Lines should not be more than 80 characters. This line is 85 characters. | 1 |
4. Other Checks
Details of other checks (click to open)
✖️ The following function name is duplicated in other packages:
-
exposurefrom mds, mlxR, netdiffuseR, RsSimulx
Package Versions
| package | version |
|---|---|
| pkgstats | 0.1.6.17 |
| pkgcheck | 0.1.2.58 |
Editor-in-Chief Instructions:
This package is in top shape and may be passed on to a handling editor
|
@ropensci-review-bot check package |
|
Thanks, about to send the query. |
|
🚀 Editor check started 👋 |
Checks for fireexposuR (v1.0.0)git hash: aacfb397
(Checks marked with 👀 may be optionally addressed.) Package License: GPL (>= 3) 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. basec (27), exp (19), sum (11), cbind (9), data.frame (9), as.data.frame (8), paste (6), ifelse (5), by (4), levels (4), matrix (4), ncol (4), all (3), factor (3), labels (3), rep (3), seq (3), class (2), match (2), names (2), round (2), unique (2), as.factor (1), for (1), if (1), length (1), max (1), mean (1), missing (1), outer (1), plot (1), rbind (1), scale (1), table (1) magrittr%>% (95) dplyrmutate (43), count (4), case_when (1), summarise (1) ggplot2element_blank (9), ggplot (7), aes (6), element_text (5), coord_sf (4), geom_col (3), labs (3), element_rect (2), expansion (2), scale_fill_manual (2), geom_bar (1), geom_hline (1), geom_vline (1), scale_x_discrete (1), scale_y_continuous (1), theme (1) terraclassify (7), crop (6), focal (4), mask (4), project (4), vect (4), extract (3), res (3), rescale (3), spatSample (2), as.polygons (1), buffer (1), crs (1), merge (1), perim (1) tidyterrageom_spatraster_rgb (6), geom_spatvector (3), rename (2), filter (1), geom_spatraster (1), scale_fill_whitebox_c (1), whitebox.colors (1) fireexposuRexposure (7), direxp (2), adjustexp (1), extractexp (1), mapexpclass (1), mapexpcont (1), multidirexp (1) gridunit (12) statswindow (6), df (4) geospheredestPoint (8) maptilesget_credit (3), get_tiles (3) utilsdata (6) ggspatialannotation_scale (4) graphicstitle (4) tidyrpivot_wider (2), pivot_longer (1) grDevicespalette (2) MultiscaleDTMannulus_window (2) NOTE: Some imported packages appear to have no 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 |
|---|---|---|---|---|---|
| 11037129203 | pkgcheck | success | aacfb3 | 7 | 2024-09-25 |
| 11037129172 | R-CMD-check.yaml | success | aacfb3 | 7 | 2024-09-25 |
3b. goodpractice results
R CMD check with rcmdcheck
rcmdcheck found no errors, warnings, or notes
Test coverage with covr
Package coverage: 93.87
Cyclocomplexity with cyclocomp
The following function have cyclocomplexity >= 15:
| function | cyclocomplexity |
|---|---|
| extractexp | 17 |
Static code analyses with lintr
lintr found the following 10 potential issues:
| message | number of times |
|---|---|
| Avoid 1:length(...) expressions, use seq_len. | 1 |
| Avoid library() and require() calls in packages | 3 |
| Lines should not be more than 80 characters. This line is 83 characters. | 4 |
| Lines should not be more than 80 characters. This line is 84 characters. | 1 |
| Lines should not be more than 80 characters. This line is 85 characters. | 1 |
4. Other Checks
Details of other checks (click to open)
✖️ The following function name is duplicated in other packages:
-
exposurefrom mds, mlxR, netdiffuseR, RsSimulx
Package Versions
| package | version |
|---|---|
| pkgstats | 0.1.6.17 |
| pkgcheck | 0.1.2.58 |
Editor-in-Chief Instructions:
This package is in top shape and may be passed on to a handling editor
|
Hi @heyairf, local checks indicate that you may not be properly ignoring the codemeta.json file in .Rbuildignore. Can you check on that for me, please? |
|
Hi @adamhsparks , I think that is now fixed? Edit: Nevermind. Standby. |
|
Thank you! I am still learning a lot about R and github. |
|
@ropensci-review-bot assign @maurolepore as reviewer |
|
Can't assign reviewer because there is no editor assigned for this submission yet |
|
@ropensci-review-bot assign @maurolepore as editor |
|
Assigned! @maurolepore is now the editor |
|
Hi @heyairf, thanks a lot for your submission. It's my pleasure to be the handling editor. Semantic tags for my commentsTo help you track my comments I'll tag them with "ml" and a numbered sequence, e.g. ml01, ml02, and so on. Comments following bullets are for you to consider -- you may or may not respond to them. Comments following check-boxes are requests for some action -- please respond. Reviewers
Editor checksI'll post my checks in a few days. I already explored the package and looks in good shape. I appreciate your kindness in sharing your work with the rOpenSci community, and your willingness to learn a bit more about R package development. In that spirit I'll share a number of suggestions to make the reviews as smooth as possible. |
|
Hi @maurolepore,
|
|
Hi @heyairf sorry for the delay. I'm close to finishing polishing the notes from my checks. I expect to finish it by Wed-Thu at the latest. |
|
Dear @heyairf, Once again, thank for sharing your great work with the rOpenSci community. Here are my checks and comments (based on the editor's template). Remember the items in bullets are just for your consideration, and the check-boxes require some action (sometimes just an explanation). The more items you can address before review the better. It will reduce the chance that reviewers will pick up in these relatively uninteresting structural issues and free them to focus on the more interesting aspects of the package. Please feel free to ask for clarifications or help. If I don't hear from you before, I'll touch base in a couple of weeks. Editor checks:
Editor commentsDocumentation
It sounds hard but all you need to do is to run
This allows "for an assessment of functionality and scope without installing the package", which should help the reviewers and users.
More importantly, it would be nice to use references to "exposure" consistently in the names of the functions. Right now I see three ways to refer to exposure:
Consider using a prefix rather than the alternatives. That way users can type the prefix and see all related functions thanks to auto-complete. Also "We strongly recommend See Function and argument naming at https://devdevguide.netlify.app/pkg_building.html#function-and-argument-naming
Fit and overlap
TestsThe tests could improve in a number of ways. I don't have first-had experience testing specifically spatial data so I'll try to find a reviewer who has (thanks for being proactive in https://discuss.ropensci.org/t/s4-data-for-testthat-and-examples/4007/5). For now I'll share some feedback that applies to tests in general.
These two test-files are the slowest: If you trully can't make tests run faster, then you may move them to a dedicated folder e.g. tests/testthat/slow and run those tests less frequently than every other test -- with References: https://testthat.r-lib.org/reference/test_dir.html and https://testthat.r-lib.org/reference/test_path.html
Here are the verbose functions and the output I see clutering the test-report: See https://ropensci.org/blog/2024/02/06/verbosity-control-packages/
Specifially, I see this code-chunk multiple times: # test data ====================================================================
set.seed(0)
e <- c(45,55,495,505) * 10000
r <- terra::rast(resolution = 100, extent = terra::ext(e))
terra::values(r) <- sample(c(0,1), terra::ncell(r), replace = TRUE)
r <- terra::sieve(r, threshold = 50, directions = 4)
haz <- terra::sieve(r, threshold = 500, directions = 4)
Here is one example from the package: expect_error(direxp(2, pt))
test_that("mapexpclass() input checks and function messages work", {
expect_condition(mapexplass(2, "loc", aoicrs)) # stopifnot() L67
expect_condition(mapexpclass(expvals, "loc", aoicrs)) # stopifnot() L69
expect_condition(mapexpclass(exp, "loc", 2)) # stopifnot() L71
expect_condition(mapexpclass(exp, "loc", aoibig)) # stopifnot() L73
expect_condition(mapexpclass(exp, "blah", aoicrs)) # matchargs() L75
expect_condition()
})
The code chunk below shows that Option 1: # https://github.com/heyairf/fireexposuR/blob/main/tests/testthat/test-multidirexp.R
test_that("multidirexp() returns object with correct class", {
expect_s3_class(multidirexp(exp, pts, plot = T), "ggplot")
expect_s3_class(multidirexp(exp, pts), "data.frame")
})
test_that("summexp() input checks work", {
expect_condition(summexp(2)) #exp: right class
expect_condition(summexp(exp, 2)) #aoi: right class
expect_condition(summexp(exp, aoi, "blah")) # classify: match arg
})Project management
# The fireexposuR directory is quite large, so cloning is relatively slow
➜ git du -hd 1 fireexposuR
240K fireexposuR/.Rproj.user
80K fireexposuR/R
20K fireexposuR/inst
76M fireexposuR/.git
16K fireexposuR/vignettes
24K fireexposuR/.github
736K fireexposuR/man
48K fireexposuR/tests
77M fireexposuR
# Almost all that "weight" comes from the .git/ directory alone
➜ git du -hd 1 fireexposuR/.git
4.0K fireexposuR/.git/branches
75M fireexposuR/.git/objects
32K fireexposuR/.git/logs
8.0K fireexposuR/.git/info
28K fireexposuR/.git/refs
64K fireexposuR/.git/hooks
76M fireexposuR/.git
|
|
Hi @maurolepore, Thank you so much for such detailed feedback! I will work on addressing these comments over the next few days and ask for further clarification if I need to. I very much appreciate all the resources you've included. |
|
Hi @maurolepore, I have attempted to addressed your feedback within my package! My replies to each of your comments are below. I expect that some may need to be addressed further, please let me know. ml01
ml02
ml03
ml04/ml08
ml05
ml06
ml07
ml08
ml09/ml10
ml11
ml12
ml13
ml14
|
|
@ropensci-review-bot check package |
|
Thanks, about to send the query. |
|
@heyairf thanks so much for your effort and your quick response. Great work. I'll start reaching out to potential reviewers.
Also ...
Warning (test-fire_exp_extract_vis.R:43:3): fire_exp_extract_vis() runs when input conditions are met
Use of .data in tidyselect expressions was deprecated in tidyselect 1.2.0.
i Please use `"class"` instead of `.data$class`The problem is in R/fire_exp_extract_vis.R#L137. You can fix using use tidyr::drop_na("class")
|
|
Sorry @maurolepore and @heyairf , the check package service is currently not working. It'll be back online in about 12 hours, at which time I'll make sure that request gets processed. Thanks |
|
@huizezhang-sherry: If you haven't done so, please fill this form for us to update our reviewers records. |
|
@heyairf we now have the second and last reviewer! Thanks for your patience. (BTW the bot posted a mistaken message requesting your response. I deleted it but if you still see it in your inbox please ignore it.) |
|
📆 @huizezhang-sherry you have 2 days left before the due date for your review (2024-12-21). |
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: 3.5
Review Comments
|
|
@huizezhang-sherry thanks so much for your review! |
|
@ropensci-review-bot submit review #659 (comment) time 3.5 |
|
Logged review for huizezhang-sherry (hours: 3.5) |
ropensci-review-bot
added
4/review(s)-in-awaiting-changes
and removed
3/reviewer(s)-assigned
labels
|
@heyairf you can now go ahead and incorporate both reviews. Thank you for being patient 🙏 https://devdevguide.netlify.app/softwarereview_author#the-review-process
|
|
Hello @maurolepore , and reviewers! |
|
Absolutely, the end of the year is inconvenient for everyone |
|
@heyairf: please post your response with Here's the author guide for response. https://devguide.ropensci.org/authors-guide.html |
|
@heyairf please don't stress about the bot's reminder ;-) |
|
Hi everyone, I just wanted to say that your comments and suggestions have been so helpful in thinking about what the documentation should look like. As I have been going through the package to incorporate your feedback to all the current functions I have realized it is quite a monumental task to go through it all and it is taking me a considerable amount of time. I have pushed a few changes to the main branch to respond to your comments with examples of the changes I am making, but I have not completed documenting all the functions yet. I am also drafting a better README file to incorporate some more of your great feedback. I have marked my responses with numbers to make them easier to respond to if required. I don’t expect you to take the time to reply until I am finished unless you want to! I will let you know as soon as that is. The only comment I would appreciate a response to if possible is number 4… if any of you could help me identify where I went wrong with that I’d be grateful. Thanks, Response to @ronnyhdez It would be helpful to clarify how coordinate reference systems are handled throughout the package, particularly whether transformations affect only the visualization outputs or also the underlying analyses. For example, the use of tiles involves the re-projection to EPSG:3857, which is necessary for visualization. Given that the target audience of this package seems to be wildfire scientists, documenting these spatial operations would enhance the functions' usability.
Also, it would be helpful to indicate the geographical scope of the implemented methods. From the references included in the README, it seems that the studies were all conducted in Alberta, Canada. If the methods are specifically calibrated for this region's characteristics, this should be explicitly stated in the package documentation to help users assess the applicability to their study areas.
While references to scientific publications provide valuable methodological context, the function documentation would be more effective if it primarily focused on usage and implementation details, with citations serving as supplementary information. This would enhance the user experience by providing immediate access to practical implementation guidance.
I would suggest adding a CITATION file (see rOpenSci guidelineshere) to help users properly acknowledge your work when using the package in their research. The checkpoiints above state that there should be a 'contribution guidelines'.
Specific thoughts
code snippets
Finally, I think the package's visualization examples would be even more engaging with terrestrial locations, as this would highlight the integration with base map tiles and provide users with examples that directly relate to typical wildfire analysis scenarios. If there are specific reasons/constraints for using oceanic locations in the examples it's fine.
Functions
There are some functions which have fixed values in their bodies. I'm wondering if those could be arguments with default values that the user can change. This is one example
Response to @huizezhang-sherry The package would benefit from more documentation on the methods and how outputs are computed. While the papers referenced provide comprehensive details, a good rule of thumb is that the package documentation(README, articles, and function reference) should be self-contained, with publication(s) referenced for further details. It would be helpful to explain how the summary in
The basemap example in
The author may consider explaining the difference of using
The use of color (red, orange, and yellow) in
Function names in the package could more clearly reflect their purposes. The package provides two types of functions: 1) those that summarize fire exposure metrics and 2) those that plot fire exposure. It would be helpful if function names can explicitly indicates their purpose. For example, standardizing visualization functions as
In summary: I’m still working on things but I am dedicating lots of time to this! If you have comments you’d like to respond to now please do. I have no problems with you waiting until I have fully incorporated your feedback into a final polished product. Thanks so much for all of your helpful feedback. :) |
|
Dear @heyairf this is to mark the start of my EiC rotation and record what I see from the perspective of that role. @heyairf I see you've done a lot of work already. I also see you intend to work more and that's great. However I suggest we first focus on just-enough to satisfy the reviewers' requests. That way we can use the context they have in their heads before they loose it. Anything beyond that is important but not urgent and I encourage you to do it after approval. In other words, when do you think you would be able to do the minimum required to satisfy the key issues the reviewers requested? No problem if you think it'll take quite some time. We do understand and offer a holding label if you still choose to wait. https://devguide.ropensci.org/softwarereview_policies.html#policiesreviewprocess The author can choose to have their submission put on hold (editor applies the holding label). The holding status will be revisited every 3 months, and after one year the issue will be closed. |
|
Hi @maurolepore , I was just coming back to let everyone know that I am satisfied with the current state of the package, and that I think all reviewer comments have been addressed. The only thing I am working on still is a final vignette that does not document the package or the functions within, but will offer advice for preparing input data. Thank you everyone for your patience, as I continue to learn how to develop an R package! |
maurolepore
added
5/awaiting-reviewer(s)-response
and removed
4/review(s)-in-awaiting-changes
labels
|
@heyairf, great news! Dear @ronnyhdez and @huizezhang-sherry, please let us know if you request further changes. Else if you are satisfied please indicate your approval using this template: |
Reviewer ResponseFinal approval (post-review)
Estimated hours spent reviewing: 1 |
Reviewer ResponseFinal approval (post-review)
Estimated hours spent reviewing: 1 Note: cool hex sticker! |
|
@ropensci-review-bot approve fireexposuR |
|
Approved! Thanks @heyairf for submitting and @ronnyhdez, @huizezhang-sherry 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
added
6/approved
and removed
5/awaiting-reviewer(s)-response
labels
|
@heyairf , @ronnyhdez, and @huizezhang-sherry, On a less automatic note, thanks so much for kindly contributing your work and experience. I look forward to seeing you around in the rOpenSci workspace on Slack. If for any reason you lack access please let me know. All the best, |
Date accepted: 2025-02-10
Submitting Author Name: Air Forbes
Submitting Author Github Handle: @heyairf
Repository: https://github.com/heyairf/fireexposuR
Version submitted: 1.0.0
Submission type: Standard
Editor: @maurolepore
Reviewers: @ronnyhdez, @huizezhang-sherry
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):
This package was developed to share methodologies from existing research by modifying/expanding existing functions. The functions automate and standardize assessments to increase access and quality assurance in the calculation for interested users.
Users interested in wildfire risk assessments including but not limited to researchers, consultants, planners, land use decision makers.
There are no other R packages that fill this role.
n/a
#652
pkgcheckitems which your package is unable to pass.all pkgcheck's are currently passing
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: