-
-
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
Submission:MtreeRing #287
Comments
Thank you for your submission @JingningShi - @annakrystalli has been assigned as the Editor. |
Editor checks:
Editor commentsHello @JingningShi. 👋 Many thanks for your submission and apologies for the delay in completing the editors checks for you! Firstly, it looks like a really cool package 😎. There's a few issues I found during the editor's checks though that will need addressing before we proceed:
goodpracticeAs mentioned above, these files need at blank line at the end of each.
Also, it's also good practice to:
Let me know when these issues have been addressed and I will begin looking for reviewers. Also feel free to reach out if have any more questions |
Thanks for the editor checks @annakrystalli . |
Thanks for checking out the package @annakrystalli . It was the first time that I had written an R package. English isn't my first language, so please excuse any mistakes. I made the following changes based on your comments:
Regarding the low test coverage you mentioned, the reason is that some functions in our package use Thanks again for looking it over and please let me know if you have any other questions. |
Hi @JingningShi , Thanks for your speedy and full response! Regarding English as a second language, don't worry at all. If there is anything I write that doesn't make sense let me know and I will try to explain in a different way. I will do the same. I've re-run all the checks & tests and most of the issues have been solved! 🎉 Regarding testing coverage, I have two suggestions to increase it:
Finally, |
Thank you for the suggestions of using
Thanks again for the review. I have learned many things throughout the editor checks. It was extremely helpful. |
Hi @JingningShi, So glad you found the suggestions useful! And well done on getting coverage up to 97%!! 😃💯 Could you please add the rOpenSci under review badge to your README?
I'll be back in touch when I have found reviewers 👍 |
We now have reviewers! Many thanks for agreeing to review @bhive01 & @rorynolan 🙏 Note a delay in the official start date (2018-04-09) to accommodate reviewers schedules. Reviewers: @bhive01, @rorynolan |
Hello @JingningShi, @annakrystalli and @bhive01 @JingningShi, great job on a really nice package. I have several improvements that I'd like you to implement but I like the package a lot, it was nice to try it out. 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
Final approval (post-review)
Estimated hours spent reviewing: 4 Review CommentsVignettes
Code
Tests
Continuous Integration
Documentation
Typos
rOpenSci packaging guidelinesSee https://ropensci.github.io/dev_guide/.
|
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
Final approval (post-review)
Estimated hours spent reviewing: 5 Review CommentsFunction NamingYou have quite the mix of function naming. Many functions use camelCase, a few use _ and a lot of them use '.'. https://ropensci.github.io/dev_guide/building.html#function-and-argument-naming suggests using snake_case which is used in a few instances.
autoDetect.Rmethod = "lineardetect" fails with example and there is no indication why.
sample.yr defaults to the current year calcRingWidth.RTwo things that bugged me when I tried to use this function:
f.img.RFor some reason when using autoDetect, I would get an empty quartz window every time I used it. I suspect the reason is in this file, but I can't see it obviously. launchMtRApp.RThis is good. Really good. Makes a really intuitive interface to the whole process. The only issue I had with this was when I wanted to stop running this app R would not respond. This was outside of Rstudio (gasp). In R-gui Mac, it just gave me the spinning color wheel of death until I force quit. My bad, but is this a known thing. I haven't had time to look. When I did the auto border search I got the following message: The app works pretty well and is intuitive. How do you save the output? nearPith.RThe first time it loaded up and I didn't see the instructions to click. I finally realized this and clicked, but when I wanted to click on the arcs it failed. visualSelect.RThe function seems to do well at removing existing borders, but when I say add = TRUE, it doesn't. Is this meant to only be run in Rstudio or something? watershed.im.RSeems to be a repository for functions. Perhaps this should be tools.R or something similar? OverallI think this is a really good fit for ropensci, but I think that perhaps it needs a bit more work to clarify what's going on with the interactive bits. I'd say it's close, but not quite there. The shiny app is awesome and one of the first examples I've come across that provide a tool to make the process of doing this sort of thing for those less familiar with the command line. |
@rorynolan @bhive01 Thanks a lot for the thorough review and great suggestions. I will address these issues and report back here. |
Sorry for the delay in the response. Thank you again for taking the time to thoroughly review the package @rorynolan @bhive01 . English isn't my first language, so please excuse any mistakes. I will explain to the best of my ability. I'll first respond to @rorynolan comments: Vignettes
Installation instructions have been removed.
Thanks for the excellent suggestion. I deleted the old vignette and wrote two new vignettes for how to use these functions from the R console. Each vignette is demonstrated with pictures. I also wrote the third vignette for how to use the shiny app. The third vignette is demonstrated with gifs, making the workflow of shiny app more understandable.
Code
The major goal of In some cases, the image data is stored in a non-standard format. When reading such a file, we found that packages To consume less memory, we use
This is a good suggestion. I am not thoughtful enough. I have included an argument Tests
I'm not sure what went wrong. When I run Continuous Integration
This is a great suggestion. I add a new CI test using AppVeyor and this CI test passes. Documentation
Thanks for this very nice suggestion. I rename
All functions in the documentation are now clickable. rOpenSci packaging guidelines
The codemeta file has been added.
Thanks for this helpful suggestion. I rename all functions (including both internal functions and exported functions) using a snake_case style. Now, most exported functions start with
The README.Rmd file has been added and listsed in .Rbuildignore.
Both HTML documentation and the _pkgdown.yml file have been added.
The CODE_OF_CONDUCT.md file has been added, and the README.md includes a link to the CODE_OF_CONDUCT.md file. Response to @bhive01 comments Function Naming
Thanks for this helpful suggestion. I rename these functions (including both internal functions and exported functions) using a snake_case style. Now, most exported functions start with
autoDetect.R
Sorry about that. This bug has been fixed. The following calls can now properly detect ring borders.
Now
When To avoid that, I have included an argument
Thank you for this thoughtful comment, it greatly improved the user-friendliness of tree ring measurements. In a modified version, if
If
calcRingWidth.R
Thanks for this useful suggestion! The unit of ring widths is now described in the function documentation. f.img.R
I couldn't reproduce that error on my machine. When I call launchMtRApp.R
My mistake. The function documentation doesn't provide a clear and detailed description of how to stop this app. To stop the app, you could go to the R console and press the Escape key. This works in both R GUI and RStudio. You could also click the stop sign icon in the upper right corner of the RStudio console. This instruction has been added to the function documentation.
This spelling mistake has been corrected.
The output of tree ring measurements can be saved in either CSV files or RWL files (a file format used in tree ring analysis). I write a vignette for how to use the shiny app. Data saving can be found in the fifth part of this vignette.
nearPith.R
I reproduced these two errors on my computer and figured out why these errors happened. It‘s my fault. The original function documentation isn't very clear so I write a vignette for
When you call
If this process is terminated prematurely before clicking once on the arc (i.e., press the Escape key on MacOS computer, or click on ‘Stop’ button from the menu of the graphics device on Windows PC), The second error occurs because the graphics device produced by visualSelect.R
Similar to When you call A complete procedure demonstrated with pictures can be found in the vignette
watershed.im.R
Thanks for pointing this out. I've now renamed |
Thanks for your detailed response @JingningShi! ✨ Over to you @rorynolan & @bhive01. Please let us know whether the implemented changes satisfy your comments. |
Hi @JingningShi, |
Thanks @rorynolan , an explanation has been added to the R documentation. In argument description:
In details:
I will do my best to maintain and improve this package 😄 |
Going through this today. 👏🏻👏🏻👏🏻 I'm happy to say that this package is a go for me now. |
Congratulations @JingningShi! This concludes the review and the package is now approved! 🚀 Let me echo the reviewers for a really great job responding to all comments and suggestions. Many thanks as well to @rorynolan & @bhive01 for your most thorough and helpful reviews. 😺Great work all around! To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them 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 interested, @stefaniebutland will be in touch about content and timing. We've started putting together a gitbook with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here. |
Actually, sorry @JingningShi, I just picked up that the package is missing a Tracking changes to the package is a requirement for an rOpenSci package. I also would expect that given the development and many improvements made to the package during review for the post-review package to have a higher version. The Have a look at the chapter on releasing packages for more details and just reach out if you need any more help with this. Almost there! |
Great thanks! @annakrystalli |
Excellent work @JingningShi! I've given you back admin rights and am looking into the badge situation. Will get back to you shortly. |
Fixed! Many thanks again @JingningShi for your submission and engagement with the review process. As @rorynolan mentioned, this is just the start of your adventures as a package maintainer, but I've invited you to the rOpenSci slack. Feel free to ask questions there if you get stuck! |
Thanks very much, @annakrystalli .Thanks also to @rorynolan and @bhive01 for such helpful and constructive reviews. This has been a fun and useful process. I have learned a lot from these comments. 😄 |
Submitting Author: Jingning Shi (@JingningShi )
Repository: https://github.com/JingningShi/MtreeRing
Version submitted: 1.2
Editor: @annakrystalli
Reviewer 1: @bhive01
Reviewer 2: @rorynolan
Archive: TBD
Version accepted: 1.3
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):
The package aims to enable R users to obtain data from scanned tree-ring images.
Anyone interested in measuring ring-width series from increment cores or stem disks, for example, meteorologists and ecologists. Scientific applications include accurate measurements of ring-width series and data correction.
yours differ or meet our criteria for best-in-category?
To our knowledge, the package measuRing can also be used for measuring tree ring widths on scanned images. Compared to measuRing, we provide two additional approaches (watershed segmentation and Canny edge detector) for the automatic identification of ring borders. Furthermore, our package provides a Shiny-based app. This beginner-friendly app allows interactive viewing and manipulation of tree ring images and data files.
Technical checks
Confirm each of the following by checking the box. This package:
Publication options
It's already there.
JOSS Options
paper.md
matching JOSS's requirements with a high-level description in the package root or ininst/
.MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: