Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.Sign up
Confirm each of the following by checking the box. This package:
Thanks for your submission @marcosci! I'm currently looking for reviewers. I have these comments/questions:
Perfect and thank you.…
On Feb 16, 2018 11:35 AM, "Maëlle Salmon" ***@***.***> wrote: @jhollist <https://github.com/jhollist> ok, thanks for the update! March the 1st? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFL8SyqLmwOuTY6ufoDmLMZA4r31u6a4ks5tVa46gaJpZM4RrS2f> .
Hi, Thanks for the reminder. I have put time aside Monday and Tuesday to get this review done. Cheers, Laura On 16 Feb 2018 16:51, "Jeffrey W Hollister" <firstname.lastname@example.org> wrote:…
Perfect and thank you. On Feb 16, 2018 11:35 AM, "Maëlle Salmon" ***@***.***> wrote: > @jhollist <https://github.com/jhollist> ok, thanks for the update! March > the 1st? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#188 issuecomment-366287026>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ AFL8SyqLmwOuTY6ufoDmLMZA4r31u6a4ks5tVa46gaJpZM4RrS2f> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFFAxGeEQEKLGiUlcp5583hUVTfLdVq-ks5tVbICgaJpZM4RrS2f> .
The package includes all the following forms of documentation:
Final approval (post-review)
Estimated hours spent reviewing: 8
The package authors have provided a much needed collection of neutral landscape models for the R platform. I was pleased to see that it was not just a repackaging of the Python nlmpy package, but also provides additional neutral landscape models as well as some utility functions for visualising these landscapes. In terms of statement of need; the README provides a good explanation of the need for the package, but I would also add that it allows users to create NLMs within a self-contained, reproducible framework.
The code is mostly compact, efficient and follows the standards for rOpenSci packages. I have some concerns around the unit tests: the tests check that the output is of the correct form (class, size etc.), but do not go for edge cases, or try to break the functions. I have noted below some specific cases where functions either broke or did not work as expected.
One way in which I would suggest for improvement of the package would be to improve the documentation. It is mostly good, but the amount of information on the functions is variable. I have commented where I think specific functions could do with more documentation below. I do, however, see this as more of a long term thing, rather than something I'd expect to be improved as part of this review process.
The package name is sensible. It is, however, not in lower case as recommended by the packaging guide. The functions and variables all follow rOpenSci guidelines.
Below I have outlined some issues I found with some of the functions. I think most of these are pretty easy fixes with the exception of
library(NLMR) x <- nlm_random(100, 100) y <- c(0.5, 0.25, 0.25) z <- util_classify(x, y) metric_area(z, c(1,2))
The problem is in the if(length(poi) == 1) statement. This is not necessary. Any input could be evaluated by the code inside of the if part. Additionally this outputs a dataframe for Proportion_Area, and not a tibble, as stated in the documentation.
or change the order of the arguments to
fBm_raster <- nlm_fBm(ncol = 20, nrow = 30, fract_dim = 1.9)
nlm_fBm(ncol = 20, nrow = 30, fract_dim = 1, user_seed = 30)
nlm_fBm(ncol = 20, nrow = 30, fract_dim = 1)
A few notes:
Thanks again to both reviewers, and nice work until now @marcosci!
Response to reviews
We decided to follow the recommendations of Laura and Jeff to split the package. This leaves us with:
@maelle, you said:
... my gut feeling says that only the nlmr package is now of interest and not the combination of landscapetools and nlmr (however this combination may look like - meta package, second onboarding issue, ...)? If I am mistaken, I will provide comments on the reviewers' remarks on the utility functions. Otherwise, I will focus for now on nlmr.
responses to @laurajanegraham comments
We combined both functions. You get the wheyed pattern now by providing the argument
We did that here #17.
Values of fract_dim between ~1.56 and ~1.99 should work now if you set the RandomFields option
Made all perfect sense, we implemented everything you said
We fixed the issue with the loop - #20.
Again, we changed everything as you recommended :) #22
We changed the name of this function, splitted it and got rid of
The two new functions are:
All that should be in #23.
We had a look at that function again and completely changed it after your comments.
It is now a direct implementation of the random cluster algorithm by Saura et al. (2000),
See our changes in #33.
We did some pieces here and there and Craig is currently going through that again. Hope that is fine with you? :)
responses to @jhollist comments
We had a look on the way
Craig is currently working on the vignettes and documentation to make it a bit more verbose,
As stated at the beginning, by splitting the package the scope of them should
We changed every function that used
First, I'd be honored to be the mascot!!! If time allows, would be happy to more than that too.
Second, very happy with the response and direction this is taking. Two thumbs up on everything and as far as my review is concerned,
Again, really nice job. Makes me happy to see landscape ecology getting better representation in R!
Response to reviews regarding the utility functions
With respect to the comments on slack yesterday, this issue is supposed to cover
Again, you can find landscapetools here:
responses to @laurajanegraham comments
Both of the functions are only internal ones, for the reclassification function.
You can now specify the number of rows and cols, see #26.
responses to @jhollist comments
I see some value in providing a font with better kerning pairs and geometrics numbers
... did I miss anything regarding the utility functions?
As discussed on twitter, I included both of you in the respective description of the
A joke I appreciated! Also probably to most likely level of involvement I can promise. I am begrudgingly well entrenched in mid-career and as such time to work on things I want to is a rare commodity. And again, nice job on these packages. I'll include a mention in a talk I am working on for US-IALE.…
On Thu, Mar 22, 2018 at 6:00 AM, Marco Sciaini ***@***.***> wrote: Response to reviews regarding the utility functions With respect to the comments on slack yesterday, this issue is supposed to cover now both nlmr and landscapetools. Again, you can find landscapetools here: https://github.com/marcosci/landscapetools responses to @laurajanegraham <https://github.com/laurajanegraham> comments util_calc_boundaries and util_w2cp Both of the functions are only internal ones, for the reclassification function. I can't see any direct use of them - if I am mistaken, we can certainly make them public. util_facetplot You can now specify the number of rows and cols, see #26 <ropensci/NLMR#26>. responses to @jhollist <https://github.com/jhollist> comments util_import_roboto_condensed is this necessary? I see some value in providing a font with better kerning pairs and geometrics numbers (compared to the default font ggplot uses). If the dependency on extrafont is not worth it in your opinion, we can definitely cut that. ... did I miss anything regarding the utility functions? General As discussed on twitter, I included both of you in the respective description of the packages - thanks a lot again :) And the mascot comment was just a joke - we would definitely benefit from your input Jeff :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#188 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFL8S6VbHkX_ly9FwR2lcsbvpbODWHf7ks5tg3YhgaJpZM4RrS2f> .
-- Jeff W. Hollister email: email@example.com cell: 401 556 4087
A few last comments from me:
Now here is the list of things you have to do before I close this issue
Welcome aboard! We'd also love a blog post about your package, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). If you are, @stefaniebutland will be in touch about content and timing.
You mean crossreferencing nlmr and landscapetools in the readmes of the packages? :)
@maelle list of things
And we would be more than happy to write a blog post
Closing this since the blog post discussion can still happen once it's closed. :-)
I forgot two points @marcosci
Glad to hear you're interested it writing a post @marcosci. Here are some technical and editorial guidelines: https://github.com/ropensci/roweb2#contributing-a-blog-post
We ask that you submit a draft via pull request at least one week before the planned publication date. At this point I have posts scheduled through April so perhaps best for you to suggest a deadline by which you would like to submit a draft.