-
-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NLMR #188
Comments
Hi Folks, was quite unsure about the fit of our package into the rOpenSci world - but we would love to get your review and platform. Best wishes, |
thanks for your submission @marcosci we've been discussing and will get back to you asap |
Editor checks:
Editor commentsThanks for your submission @marcosci! I'm currently looking for reviewers. I have these comments/questions:
Reviewers: @laurajanegraham @jhollist |
Oops indeed! 🙀 |
@laurajanegraham @jhollist thanks for accepting to review this package! 😺 Your review is due on 2018-02-21. Please find here our reviewing guide and the review template. |
@laurajanegraham @jhollist just a friendly reminder that your reviews are due on 2018-02-21 😸 |
Starting to look at it now! I do have a question. Next week is crazy for me as it is school vacation week. OK for a 7-10 day extension? |
@jhollist ok, thanks for the update! March the 1st? |
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" <[email protected]>
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>
.
|
Thanks a lot @laurajanegraham! 😃 |
@marcosci the branch that's stable for review is the master branch right? I see recent activity in the repo. 😉 |
Yup :) But develop should also be stable, master contains only the CRAN versions :) |
Ok, so which one should the reviewers review? Can you confirm you're not undertaking any big changes in the package at the moment? Thank you for your quick answer! 😃 |
master makes more sense maybe, yes. |
OK thanks. If a big change is planned now, it might make sense to put the review on hold real quick, but as you say it's a plan for the future then that's fine. |
I think this is rather something for the next couple of months, so the review should not be affected. |
Hi @maelle . It's taking me a little bit longer to do this review than I anticipate. I don't have too much left to do, but would you mind if I get it to you by Friday instead please due to other commitments? Thanks! |
No problem, thanks for your work & this update! |
Package ReviewHi @maelle and @marcosci, here is my review. I really like this package, and reviewing it has given me a good opportunity to get to know the features in more depth, so thanks both :)
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 8 Review CommentsThe 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. FunctionalityBelow 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
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@laurajanegraham and @jhollist - thanks again for your 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: In 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.
Check: #25
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,
Check: #28
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 functionsWith respect to the comments on slack yesterday, this issue is supposed to cover Again, you can find landscapetools here: https://github.com/marcosci/landscapetools 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? GeneralAs 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 protected]
cell: 401 556 4087
|
All looking great @marcosci! |
Thanks a lot for your work @marcosci and thanks @jhollist & @laurajanegraham for very productive reviews! 🎊 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. |
Yeah 🙌 @maelle comments
You mean crossreferencing nlmr and landscapetools in the readmes of the packages? :)
Done.
Done. @maelle list of things
And we would be more than happy to write a blog post 😄 |
Thanks! Yes I meant cross-referencing with a short blurb explaining how they relate to each other. I've now made you an admin of both repositories (so you can access the repo description again for instance) and will activate them on Appveyor. |
Appveyor badges
Closing this since the blog post discussion can still happen once it's closed. :-) |
I forgot two points @marcosci 🙈
|
I am here to serve 😜 all done. |
😂 thx a ton! |
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. |
Hi @stefaniebutland and thanks :-) Would the last couple of days in April be fine to hand in the draft for the blog post? |
Yes @marcosci end of April for a draft is good. Tentative publication date is Tues May 8 so latest to submit draft is May 1st. Thanks! |
Summary
What does this package do? (explain in 50 words or less):
Paste the full DESCRIPTION file inside a code block below:
URL for the package (the development repository, not a stylized html page):
Please indicate which category or categories from our package fit policies this package falls under *and why(? (e.g., data retrieval, reproducibility. If you are unsure, we suggest you make a pre-submission inquiry.):
Who is the target audience and what are scientific applications of this package?
Are there other R packages that accomplish the same thing? If so, how does
yours differ or meet our criteria for best-in-category?
If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.
Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.Detail
Does
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings:Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
If this is a resubmission following rejection, please explain the change in circumstances:
If possible, please provide recommendations of reviewers - those with experience with similar packages and/or likely users of your package - and their GitHub user names:
The text was updated successfully, but these errors were encountered: