-
-
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
New submission: tidyhydat #152
Comments
Results of
|
thanks for your submission @boshek - We're discussing this now and will get back to you soon |
Editor checks:
Editor commentsagain, thanks for your submission @boshek Thanks for including Seeking reviewers now. Reviewers: @ldecicco-USGS @lawinslow |
NOTE: Version update since original submission. |
Reviewers are @ldecicco-USGS and @lawinslow - thanks for reviewing! due date above |
😸 @ldecicco-USGS / @lawinslow reminder that your reviews are due in 1 week 😸 |
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 CommentsOverallThe Overall, the scientific computing world requires more tools like these. Many a student, reseracher, or manager I do have a few reservations and suggestions about the current implementation of One issue I encountered was the database download, with personal handling of the database path, is awkward. It seems that much of the work Another overarching issue (which is somewhat subjective) is to the outside reviewer, the choice of function capitalization DocumentationFirst thing I noticed were the typos and grammatical errors on the Github README. I'd suggest reviewing it for I would suggest you build and commit your Rd files to github. Otherwise, devtools::install_github Line 29 zzz.R I would drop the #' comments which results in R trying to build an Rd file for .onLoad. Lastly. I would consider revising your tidyhydat-package documentation. This is frequently the first interaction On the overview side, I'd recommend, if possible, pointing to further HYDAT documentation. A lot of the documentation here There seems to be no indication of the units for any of the data. The only unit I could find is in the DATA_TYPES Lastly, a natural question that comes to my mind is, "are there other data"? How about temperature? Or Nutrients? Or FunctionalityOne thing about the examples. Using devtools I can run them, but a bunch of them are hard-coded Example for
I found that this same function failed on the testthat unit tests as well. Overall, I look forward to the tidyhydat package being available on CRAN and using its functionality to |
Thank you for this review @lawinslow. At first glance all changes suggested seem entirely appropriate. I will wait for @ldecicco-USGS's review, however, to ensure there are no conflicting changes requested. |
👍 Sounds great! |
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 CommentsRandom train-of-thoughts: LOVE the hex icon...totally jealous.
That path does not automatically work for Windows users I think. You might want to clarify that you will probably want a .Renviron file in the same directory as your RStudio project (assuming you use RStudio)...or something like that. In I'm not sure CRAN will be able run those examples (in the Generally, I'm a big fan of every exported function having at least one functional example. It helps immensely, especially when teaching new R users. A package such as this could be very useful for new R users with a flavor for Canadian hydrology. Always good to explicitly state the units for the data. Some might be obvious, but some not. I agree with Luke that it was frustrating to have the examples use a path to the database that I did not use. If there was a cleaner way to deal with this (he gave a few example packages to check out), that would make life for the user easier. You could probably have the examples work if you pointed them to:
of course...you'd need to adjust stations and what-not that are in the tiny db. It would be really cool to have (maybe another) vignette that works through a complete analysis. Maybe from looking for data, to getting the data, to visualizing the data. Alternatively, maybe expanding the help files a bit to describe more completely what is returned from each function/data set. It wasn't obvious to me until I ran the function and looked at the result. Cleaning up the style (some caps, some name_function) would be great. Great job on using I noticed
Running the tests locally, I got:
At least for the Notes on the implications of focusing on "tidy":
I see what was meant is that it converts the data from messy to tidy....but just a side not that it would be cool get some hydro tidy tools too....maybe another day! The IN CONCLUSION: |
Thank you to the reviewers. Below are some clarifications or questions. For dealing with the full review I will provide a more lengthy response addressing each comment. This is going to vastly improve the package. Some general questions/clarifications:
@ldecicco-USGS comments:
This is a good point and raises another. Could you, @sckott or @lawinslow comment on artificial data codes in general? For example would the preference be to have |
Weird. I now have no issue getting them. I swear there was a glitch in the matrix or something. Feel free to disregard.
Sure. Fair point. But I'll point out that "one thing" might refer to "all useful data pertaining to the hydrologic monitoring network". I mainly pointed it out because I was thinking about the type of data researchers might want that connects with these data. Data you might classify as "metadata" definitely came to mind. Of course one person's metadata is another person's independent package.
Yes, my preference is for the lowercase version. |
@lawinslow @ldecicco-USGS @sckott Two questions:
Or would it better to have examples that use the full hydat database like this:
Any opinions on which approach is less annoying? The "get started right away" approach or the "learn it how I am going to use it" approach?
|
hi @boshek , i can't decipher this text above
can you clarify? |
@sckott Updated the offending lines. Does that help? |
If I were you, I'd be careful with the ambiguity of a "working" function without downloading the database. i.e., would it be possible for the user to see that it is working via the example, and then try using it in a fully-functional manner and simply not get any data? That's definitely the mistake I would make. Immediately download the package, not read the documentation, and start trying to run the examples. Seeing they run, I would then try to expand to all sites (and start plotting data), but find that I could only get data for one or two sites. Just a thought and a user-case (non-documentation reader but example user) that you might want to consider. |
For function names, prefixes are a good idea to avoid namespace conflicts with other packages. Looking at your exported fxn names, they don't strike me as too likely to conflict, but some might, e.g., |
👍 yup, ship it! |
Agreed! |
Approved! Thanks again for your submission @boshek
|
@sckott Thanks! A few questions:
|
Also this should probably be changed to "tidyhydat" |
@boshek Sorry about that, forgot about the JOSS. Will ping back soon with some thoughts. team name changed. I like that idea for a blog post! The community would really benefit from that I think. @stefaniebutland thoughts on his idea for a post? |
@sckott Great! One other question - I am working through something right now and would prefer to have it all sorted out prior publicizing a release. How does ropensci decide when to publicize it's releases? Should only take a day or so to figure this out. |
What do you mean by publicize a release? Do you mean to CRAN or on twitter? or somewhere else? |
I guess mostly twitter. When does that happen or it is automated? |
If you're thinking of this account https://twitter.com/roknowtifier that is an automated bot. It sends a tweet when there is a new package on CRAN or a new version of an existing package on CRAN - so if the pkg is not on CRAN yet, there's no tweets about the pkg. In ropensci news https://ropensci.github.io/biweekly/ it's a pattern similar to the twitter bot where we mention new packages on CRAN and new versions on CRAN. |
Sam! @boshek I'm thrilled for so many reasons - your work to get BC gov't to allow this package to be hosted at rOpenSci, the fact that they have allowed this (Leadership!!), and that you'd like to contribute a blog post. Thank you!!
This is 💯 a great theme for the post. As Scott said, the community would really appreciate and benefit from this perspective. It shows the value you gained by doing something (submit to review and incorporate the feedback) that takes more work on your part than just using the code and moving on to the next thing on your to-do list. Would also be great to give a shoutout to BC Gov't (your dept) for showing leadership in submitting code for review and making it open. Feels like a good PR opportunity for your department (but I'm an idealist). Here are editorial and technical details for submitting a draft post: https://github.com/ropensci/roweb2#contributing-a-blog-post. What do you think of Nov 28 deadline for submitting draft? We would publish Dec 5 or 12. Ping me when draft is submitted and I'll review it and give feedback. |
regarding publicizing, I tweet about new blog post from @ropensci when it goes up |
@boshek for JOSS thing, can you
|
What do @sckott , @lawinslow , @ldecicco-USGS feel about having their names listed on this package as reviewers a la: https://github.com/ropensci/rrricanes/blob/cbb39aecec6dd82b78c80a81a72408e8c177de77/DESCRIPTION#L10-L12 Any objection? |
nope. |
thx, but no need to include me @boshek - laura and luke did the lion's share |
@boshek Question: I see that you put this on CRAN with the |
@noamross No snafus! Some UTF-8 string notes that I haven't figured out how to deal with yet but the |
Removing the links is particularly important because I actually linked to wrong issue. GAH! |
@noamross @sckott Any ideas on why the rOpenSci review badge shows "Peer reviewed" as the badge but when rendered in the |
@boshek I suspect that might have to do with GitHub caching? I've set the badges to expire immediately. I'll look into this and get back to you. |
@karthik Yep you are right. Caching was the issue. Thanks. |
Summary
What does this package do? (explain in 50 words or less):
tidyhydat
provides functions to extract historical and real-time national hydrometric data from Water Survey of Canada data sourcesPaste the full DESCRIPTION file inside a code block below:
URL for the package (the development repository, not a stylized html page): https://github.com/bcgov/tidyhydat
Please indicate which category or categories from our package fit policies this package falls under *and why:
tidyhydat
retrieves data from the Water Survey of Canada datamart via thedownload_realtime_dd()
function from http://dd.weather.gc.ca/hydrometric/csv/.download_realtime_ws
provides access via thehttr
package to a new password protected web service provided by Environment and Climate Change Canada (ECCC). Differences between the download realtime functions are outlined heretidyhydat
's exported functions provide easy access to ECCC's publicly available and open-licenced HYDAT sqlite3 database (http://collaboration.cmc.ec.gc.ca/cmc/hydrometrics/www/) of historical river and lake levels in Canada. HYDAT is updated quarterly and is distributed via the Canadian Open Government Licence. HYDAT is not included with this package because it is regularly updated and because the file size is prohibitively large (>1GB) for inclusion in an R package. Rathertidyhydat
provides thedownload_hydat()
function that downloads it for the user.Who is the target audience?
Are there other R packages that accomplish the same thing? If so, how does
yours differ or meet our criteria for best-in-category?
tidyhydat
.tidyhydat
documentation is more comprehensive than both packages.tidyhydat
provides added functionality not present in both packages. NeitherHYDAT
norhydatr
provide any functionality to access the web service andhydatr
does not access any real-time datamart data.tidyhydat
is to provide data in a tidy format. This conceptual data science goal provides a clear objective that is missing from both other packages.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:
Taken from the list reviewers as folks that might be appropriate:
The text was updated successfully, but these errors were encountered: