Closed mikemahoney218 closed 2 years ago
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type @ropensci-review-bot help
for help.
While it's not relevant to this issue, I do wonder if has [continuous integration](https://ropensci.github.io/dev_guide/ci.html), including reporting of test coverage using services such as Travis CI, Coveralls and/or CodeCov.
should be updated to remove Travis now that rOpenSci recommends moving away from Travis?
https://devguide.ropensci.org/ci.html?q=travis#travis-ci-linux-and-mac-osx
git hash: 8e9ea053
Important: All failing checks above must be addressed prior to proceeding
Package License: MIT + file LICENSE
This package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing.
The package has: - code in R (100% in 21 files) and - 1 authors - 3 vignettes - 1 internal data file - 5 imported packages - 27 exported functions (median 16 lines of code) - 37 non-exported functions in R (median 26 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 | 21| 82.3| | |files_vignettes | 3| 92.4| | |files_tests | 20| 96.3| | |loc_R | 1036| 69.0| | |loc_vignettes | 566| 81.1| | |loc_tests | 1431| 91.2| | |num_vignettes | 3| 94.2| | |data_size_total | 144| 57.0| | |data_size_median | 144| 56.3| | |n_fns_r | 64| 64.2| | |n_fns_r_exported | 27| 75.5| | |n_fns_r_not_exported | 37| 58.9| | |n_fns_per_file_r | 2| 31.3| | |num_params_per_fn | 3| 33.6| | |loc_per_fn_r | 20| 61.5| | |loc_per_fn_r_exp | 16| 38.0| | |loc_per_fn_r_not_exp | 26| 73.5| | |rel_whitespace_R | 8| 48.9| | |rel_whitespace_vignettes | 23| 73.7| | |rel_whitespace_tests | 26| 92.5| | |doclines_per_fn_exp | 37| 45.3| | |doclines_per_fn_not_exp | 0| 0.0|TRUE | |fn_call_network_size | 52| 67.1| | ---
Click to see the interactive network visualisation of calls between objects in package
goodpractice
and other checks#### 3a. Continuous Integration Badges [![R-CMD-check](https://github.com/mikemahoney218/unifir/workflows/R-CMD-check/badge.svg)](https://github.com/mikemahoney218/unifir/actions) **GitHub Workflow Results** |name |conclusion |sha |date | |:--------------------------|:----------|:------|:----------| |lint |success |8e9ea0 |2022-03-24 | |pages build and deployment |success |16409d |2022-03-24 | |pkgdown |success |8e9ea0 |2022-03-24 | |R-CMD-check |success |8e9ea0 |2022-03-24 | |test-coverage |success |8e9ea0 |2022-03-24 | --- #### 3b. `goodpractice` results #### `R CMD check` with [rcmdcheck](https://r-lib.github.io/rcmdcheck/) rcmdcheck found no errors, warnings, or notes #### Test coverage with [covr](https://covr.r-lib.org/) Package coverage: 86.83 #### Cyclocomplexity with [cyclocomp](https://github.com/MangoTheCat/cyclocomp) The following functions have cyclocomplexity >= 15: function | cyclocomplexity --- | --- action | 26 find_unity | 16 #### Static code analyses with [lintr](https://github.com/jimhester/lintr) [lintr](https://github.com/jimhester/lintr) found the following 10 potential issues: message | number of times --- | --- Lines should not be more than 80 characters. | 10
|package |version | |:--------|:---------| |pkgstats |0.0.3.94 | |pkgcheck |0.0.2.275 |
Processing may not proceed until the items marked with :heavy_multiplication_x: have been resolved.
@ropensci-review-bot assign @melvidoni as editor
I'm sorry human, I don't understand that. You can see what commands I support by typing:
@ropensci-review-bot help
@ropensci-review-bot assign @melvidoni as editor
Assigned! @melvidoni is now the editor
@ropensci-review-bot seeking reviewers
Please add this badge to the README of your package repository:
[![Status at rOpenSci Software Peer Review](https://badges.ropensci.org/521_status.svg)](https://github.com/ropensci/software-review/issues/521)
Furthermore, if your package does not have a NEWS.md file yet, please create one to capture the changes made during the review process. See https://devguide.ropensci.org/releasing.html#news
@ropensci-review-bot assign @vinhtantran as reviewer
@vinhtantran added to the reviewers list. Review due date is 2022-04-20. Thanks @vinhtantran for accepting to review! Please refer to our reviewer guide.
@vinhtantran: If you haven't done so, please fill this form for us to update our reviewers records.
@ropensci-review-bot assign @wjones127 as reviewer
@wjones127 added to the reviewers list. Review due date is 2022-04-29. Thanks @wjones127 for accepting to review! Please refer to our reviewer guide.
@wjones127: If you haven't done so, please fill this form for us to update our reviewers records.
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
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).Estimated hours spent reviewing: 8 hours
I am not a Unity user or game developer, so I have to rely to the “Why do this?” section in the README to understand why the package is needed. The author pointed out two motivations: the use of closed-source tooling in current research and the potential of bias for personal preference. The first motivation is intriguing in that the package has the potential to encourage researchers to publish open-source and reproducible source codes for their studies. However, I hope the author can elaborate on the second point in terms of how the package would assist with the aforementioned issue.
The source code was well-written and packaged. I have nothing to say about how the code was written and structured. It’s not surprising given that this is not the author’s first submission to rOpenSci.
The vignettes are simple to follow and put into action. By following the 102 vignette, I was able to create a toy working example in Unity. I understand that the CRAN check and GitHub actions limit your ability to showcase more sophisticated examples with your assets, but I’d love to see one that highlights all package’s features, perhaps from a blogpost? It’s only a suggestion.
Another point I’d like to make is about the package’s ongoing development. I cloned the package and wrote a bunch of comments on some inconsistencies in your function documentation a few weeks ago, only to discover that the majority of them have been fixed in your most recent commits, which were made a few days ago. Although the majority of them are minor changes, I would recommend that you include a note in the Review report to let us reviewers know which version we should work on.
Finally, congratulations on a job well done. The review process taught me a lot, including new information about the R6 object in R.
I’ve included some detailed comments below. Some of them may be obsolete in your most recent commit because I did not have time to revise them all.
instantiate_prefab()
in add_default_player()
.add_texture()
, it states “A ‘unifir_script’ object”, then in add_prop()
, it states “A [unifir_prop] object”.find_unity()
Description is mixed with Details. They are not continuous. Either move everything to the Description or the Details sections.library(unifir)
in the setup chunk at top of the pages should be hidden (echo = FALSE
). Readers should see it right before the first call to the package’s function, similar to what you have in “Unifir 101”, instead of out of nowhere at the top of the pages.waiver()
and others to ensure rendering on CRAN could be put in the comment on top of the function call instead of the text below the code. Also, make sure to note all those “blank code”. You miss one waiver()
notice at Action section of 102.get_asset()
calls add_players()
which is no longer available in the latest commit.Thank you so much @vinhtantran ! I apologize for having you review a "moving target" -- I was doing a standard update across all my packages and completely forgot this one should be "frozen". I'll follow up with fixes and replies once our other review is in!
I'm about half-way through my review, but in the meantime I've posted an issue in the repo:
Added two other issues:
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
The package includes all the following forms of documentation:
URL
, BugReports
and Maintainer
(which may be autogenerated via Authors@R
).Estimated hours spent reviewing:
Overall this package is very good. The code style and level of testing are excellent.
I worked through the vignettes successfully, and then tried to create a simple scene of my own. I've reported the major issues I encountered in that process, which I think ought to be addressed (most already have 💯 ):
In addition, I noticed a few smaller issues while reviewing the package:
find_unity()
docs are a little funny: the enumerated list is pushed down to details, so it's a little confusing.add_default_tree()
adds trees that are 90 degrees from vertical.Thank you @vinhtantran and @wjones127 for the reviews! Please, @mikemahoney218 keep us posted on how you are advancing with this.
Will do! I've got a feature branch with changes in response to these reviews open at https://github.com/mikemahoney218/unifir/pull/5 , and I expect to have addressed all comments in the near future -- I'll update here once I've done so.
Thanks everyone!
Hi all! I believe I've addressed all reviewer comments in a branch on the unifir repo, https://github.com/mikemahoney218/unifir/tree/response_to_reviewers (diff at https://github.com/mikemahoney218/unifir/pull/5/files , NEWS at https://github.com/mikemahoney218/unifir/blob/response_to_reviewers/NEWS.md ). Thank you so much for your comments, the package is looking a lot better as a result!
Thanks so much for your review! I'd like to include you in the DESCRIPTION as a reviewer -- would you be able to tell me what I should list for "given" and "family" names? I don't want to infer incorrectly!
pairs when referring to function and argument names. (AND) [x] The object names displayed are not consistent. For example, in add_texture(), it states “A ‘unifir_script’ object”, then in add_prop(), it states “A [unifir_prop] object”.
`, functions by [ (which should generate links), and I now attempt to always refer to both the relevant class and constructor function (so the given example now reads "A
unifir_scriptobject created by [make_script]" and "a
unifir_prop` object created by [unifir_prop]".Thank you so much for your review! Below I'm only addressing the comments on this issue; I've dealt with the larger issues on their own tickets in the unifir repo. I'd like to include you in the DESCRIPTION as a reviewer, as well -- should I list you as Will Jones or something else?
find_unity()
docs are a little funny: the enumerated list is pushed down to details, so it's a little confusing.
add_default_tree()
adds trees that are 90 degrees from vertical.
Thanks for the response @mikemahoney218. @vinhtantran and @wjones127 can you please review the response and see if your comments were addressed?
@mikemahoney218 I can confirm that all my comments have been addressed. I left a few optional follow-up comments in my final review of the changes here: https://github.com/mikemahoney218/unifir/pull/5#pullrequestreview-957918764
Great work!
I'd like to include you in the DESCRIPTION as a reviewer, as well -- should I list you as Will Jones or something else?
Yes, "Will Jones" is good to list me as. Email is willjones127 at gmail, if you need that.
All of my comments have been addressed. Great work @mikemahoney218.
Response to @vinhtantran
Thanks so much for your review! I'd like to include you in the DESCRIPTION as a reviewer -- would you be able to tell me what I should list for "given" and "family" names? I don't want to infer incorrectly!
Please use "Tan Tran" with the email address of vinhtantran at GMail.
Hello all @vinhtantran and @wjones127 thank you for the review!
@mikemahoney218 since @wjones127 left a final review, please address that and let me know once it is done so I can approve..
@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/521#issuecomment-1110187724 time 8
Logged review for wjones127 (hours: 8)
@ropensci-review-bot submit review https://github.com/ropensci/software-review/issues/521#issuecomment-1100715168 time 8
Logged review for vinhtantran (hours: 8)
Hi @melvidoni ! All of the last changes have been made (all resolved as part of https://github.com/mikemahoney218/unifir/pull/5#pullrequestreview-957918764 and https://github.com/mikemahoney218/unifir/issues/7 ) so I believe we're all set here.
Thanks!
@ropensci-review-bot approve unifir
Approved! Thanks @mikemahoney218 for submitting and @vinhtantran, @wjones127 for your reviews! :grin:
To-dos:
@ropensci-review-bot finalize transfer of <package-name>
where <package-name>
is the repo/package name. This will give you admin access back.pkgdown
website and are ok relying only on rOpenSci central docs building and branding,
pkgdown
website with a redirecting pagehttps://docs.ropensci.org/package_name
URL
field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
codemetar::write_codemeta()
in the root of your package.install.packages("<package-name>", repos = "https://ropensci.r-universe.dev")
thanks to R-universe.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. She 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 finalize transfer of unifir
Transfer completed. The unifir
team is now owner of the repository
@melvidoni I'm not sure if there's an expected lag, but the bot doesn't appear to have added me to the unifir team (and as such I don't yet have admin on the repo)
Hello @mikemahoney218. You'll have to wait so I can ask about the bot. I believe it should have created the group. Give me a couple days, please.
No worries! It did create the team -- https://github.com/orgs/ropensci/teams/unifir -- but didn't add me. I'm on the terrainr team ( https://github.com/orgs/ropensci/teams/terrainr ) so I don't believe there's any issue with my MFA setup. No rush at all!
I had to approve you, weird, I'm exploring. In any case you've now been added to the team @mikemahoney218!
Awesome, thanks @maelle ! In case it helps debugging, I manually requested access to the team after the bot didn't add me, I didn't get added without approval.
I believe that means the repo is transferred and we're all good to go! Thank you so much everyone!
You might be the first "repeat" submitter since we started using the bot, we'll investigate. Thanks for catching this bug and your patience :grin:
Date accepted: 2022-05-01
Submitting Author Name: Mike Mahoney Submitting Author Github Handle: !--author1-->@mikemahoney218<!--end-author1-- Repository: https://github.com/mikemahoney218/unifir Version submitted: 0.1.0 Submission type: Standard Editor: !--editor-->@melvidoni<!--end-editor-- Reviewers: @vinhtantran, @wjones127
Due date for @vinhtantran: 2022-04-20 Due date for @wjones127: 2022-04-29
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): Following discussion in #508, I believe this package is within scope for the following reason:
This package provides bindings to the Unity video game engine API from R, for the production of 3D/VR "environments" entirely from R code. It enables producing a new type of visualization in an efficient and reproducible manner, and provides specific methods for visualizing spatial data stored in standard spatial formats. However, I could understand if the package itself is too general for rOpenSci in particular; while we are designing it for research applications, Unity is not explicitly scientific software.
The target audience is anyone looking to produce data-driven immersive virtual environments in Unity, with a primary focus on visualizing natural environments. I work directly with ecologists and landscape architects, and so the initial feature set is oriented towards making the package useful for that audience.
Immersive virtual environments are an active area of research (see for instance https://www.mm218.dev/papers/vrs_2021.pdf, https://joss.theoj.org/papers/10.21105/joss.04060, https://link.springer.com/article/10.1007/s42489-020-00069-6 ). At the moment, the current standard for the field is producing hand-designed "artistic" environments tailored for each purpose, which makes assessing this visualization method difficult. Our aim with this project is to produce a standard set of tooling for creating immersive virtual environments, making it both easier to produce visualization "bases" for applications and giving us a way to produce reproducible visualizations as a baseline for assessing visualization effectiveness directly.
I'm not aware of any. The closest is likely the rayverse (rayshader/rayrender), which makes fantastic 3D visualizations in R directly, without incorporating Unity; the incorporation of Unity makes adding player controllers (for interactive exploration of the environment) much easier.
N/A
508
pkgcheck
items which your package is unable to pass.x These functions do not have examples: [check_debug, create_if_not].
These functions are not exported, which given my past experiences with CRAN implies that they should not have examples (as apparently examples are run for non-exported functions on test machines).
Technical checks
Confirm each of the following by checking the box.
This package:
Publication options
[x] 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
- [ ] 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