-
-
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
weathercan #160
Comments
👋 @steffilazerte - thanks for your submission! We'll discuss fit for rOpenSci and get back to you soon. |
ah woops, sorry. forgot we already covered this in presubmission thing. Will assign an editor soon |
Editor checks:
Editor commentsThanks for your submission @steffilazerte !! Goodpractice output for you and reviewers to consider: It is good practice to
✖ write unit tests for all functions, and all package code
in general. 94% of code lines are covered by test cases.
R/interpolate.R:64:NA
R/interpolate.R:65:NA
R/interpolate.R:84:NA
R/interpolate.R:142:NA
R/interpolate.R:151:NA
... and 27 more lines
✖ avoid long code lines, it is bad for readability. Also,
many people prefer editor windows that are about 80 characters
wide. Try make your lines shorter than 80 characters
R/interpolate.R:62:1
R/interpolate.R:63:1
R/interpolate.R:78:1
R/interpolate.R:97:1
R/interpolate.R:119:1
... and 93 more lines
✖ avoid sapply(), it is not type safe. It might return a
vector, or a list, depending on the input data. Consider using
vapply() instead.
R/interpolate.R:103:18
R/interpolate.R:165:18
R/interpolate.R:182:18
R/weather.R:367:10
R/weather.R:369:30
... and 4 more lines
✖ fix this R CMD check NOTE: Note: found 26 marked UTF-8
strings Seeking reviewers now. Reviewers: @joethorley @softloud |
Reviewers assigned. They are: @joethorley @softloud |
Package Review
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 5 Review CommentsThis is a useful package to locate Environment Canada weather stations, A statement of needThe README definitely requires a brief discussion of how the current package differs from ExamplesAn example is lacking for READMEThe demonstration of the use of VignettesPackaging GuidelinesThe function naming should follow the suggested The README lacks citation information. The two most recent NEWS.md headings lack dates. DESCRIPTIONThe DESCRIPTION file should include the Date of the current version. Coding StyleThe readability would be improved by breaking the bigger functions into subfunctions but this could be alot of work and is not necessary for acceptance of the package. |
Thanks @joethorley, this all looks pretty straightforward, thanks for your speedy review! I'll start looking into these changes. |
@softloud please get your review in by next wednesday, thanks! |
Thanks for the prompt. I am impressed by the efficiency of rOpenSci! Hoping to get it done this weekend. |
Package Review
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: Review CommentsI appreciate the opportunity to review a package like this; I learnt a lot from its well-designed structure. I like the idea of a VignettesNow I don't know if this is something to do with my inexperience, but I couldn't get the vignettes to run. In fact, I struggled to access them at all. Just in case this is an OS thing, I'm trying this out on a Macbook Air High Sierra i5 from 2014. Let me know if there's any other information you need from me.
ExamplesAs @joethorley noted, there are one or two functions that are missing an example. When I went back through I think it was just TestingThere was one failure when running tests. This looks very minor, if it is anything to worry about. The tests also appear comprehensive.
Other testsI ran various other ad hoc tests, including the |
thanks for your review @softloud ! @steffilazerte both reviews are in, continue discussion here ... |
FWIW I built the vignettes successfully on OSX High Sierra using |
Great thanks to both of you. I'm not sure what's going on with that test but I'll take a look. |
At long last! Apologies for the delay, and many thanks to all reviewers for your time and effort, it is really appreciated. I have made changes and responded to your comments/suggestions below, quoting the commit the change was made in. Response to @joethorleyA statement of need
Examples
README
Packaging Guidelines
DESCRIPTION
Coding Style
Response to @softloud
Extra Changes
|
Thanks for your response @steffilazerte - @softloud and @joethorley let us know if you are happy with the authors response to your reviews and if there's any others aspects you have comments on |
@sckott I am happy with the authors response - I have no further comments |
thanks @joethorley |
thanks for reminding me @steffilazerte was waiting on the other reviewes response. but we'll pass on that. I'm taking a quick look to see if there's anything else |
Just a few things I noticed:
of the above, can you take care of the 1st and 3rd before we move on? |
Absolutely. I'll get back to you soon (may take a day or two, depending when I get a free moment). Thanks! |
Okay, I've removed the Date from the DESCRIPTION file, shortened all the too long lines and switched |
Great, thanks much @steffilazerte taking a peek |
Approved! Thanks again for your submission @steffilazerte
|
@steffilazerte closing issue now b/c approval, but can continue any discussion here still |
Okay sounds good, it asks me which teams should have access, I assume only two teams: Leadership and weathercan?
|
yep, thanks
Great, @stefaniebutland will get in touch with you
|
Excellent! You can find some technical and editorial guidelines here: https://github.com/ropensci/roweb2#contributing-a-blog-post. I have a spot open for a post on Tues Mar 6. We typically ask for a draft via pull request at least a week in advance so we can review it. What do you think about submitting a draft by Tues Feb 27th? If you submit your draft earlier, we might publish earlier if another post is delayed. Thank you for being willing to do the extra work for a post. I hope it will get more eyes on your work. |
We need a couple of days to sort things out, but I can let you know early next week if we can make that date, etc. Thanks! |
But in trying to get weathercan set up with zenodo, I don't seem to have access to the weathercan repository in order to enable the hook. I also don't have access to the settings tab on weathercan anymore where the webhook details are stored. Is this something you need to enable? Also can we get @boshek added to the weathercan team? |
🚀 thanks zenodo integration turned on and you and sam now have admin rights on repo |
Hi @steffilazerte. Further thoughts on a date to submit a draft blog post? draft by Tues Feb 27th for publication the following week? |
Sorry for the delay, we've been chatting about how to go about this and have some great ideas. However, I don't think we'd be able to make the Feb 27 deadline. Would it be possible to go for a later posting? When would that be? |
No problem - was not even a delay. Name your preferred date to submit a draft and we'll work with that. |
Okay, we're sitting down on Monday to work it all out, then we should have an idea of when we can make it happen by. Will keep you posted! |
Hi @stefaniebutland Okay, I think we've got it sorted :) Sam's got a lot going on, so I'll write the blog post solo. As It's a pretty simple idea, but I find that it's usually the loading and combining data sets that most people get hung up on. Would this work? I'm not certain what style or length you're looking for. I think I could get you a draft by March 9th, if that would work! |
Your idea sounds good. You probably have the best handle on what users will want to do with the package and always good to write about something you're keen to share. Using the integration examples you suggest should allow others to figure out how to integrate with other data that interest them, so once you've written up your examples, see if there's a place for a paragraph pointing out any generic issues people should pay attention to in integrating with other datasets. Maybe Sam can have a read-through before you submit pull request, as an informed second pair of eyes. I have this post in my calendar to be published Tues Mar 13th, so could you submit draft by Mar 6th? |
@stefaniebutland @steffilazerte Happy to read! |
Good idea about the generic issues, I'll definitely include that section. Glad you're up for the read through Sam! I should be able to have the draft by March 6th, no problem. |
re: length. Shorter is probably better, to keep people's attention. Have a look at other posts and see which ones you like: https://ropensci.org/tags/review/ + https://ropensci.org/tags/onboarding/ |
@steffilazerte I'm checking in to see if you still expect to have draft submitted by next week. If you're feeling ambitious, I have an open slot to publish on Tues March 6th, but that would mean submitting a draft this week. No obligation. |
@stefaniebutland I'm definitely on track for next week. I might be able to do this week, well see. I just sent the draft to Sam, so it'll depend on when he comes back to me and with what suggestions. |
Summary
weathercan
helps users access historical weather data from Environment and Climate Change Canada (ECCC). Data is freely available from the ECCC website, but downloading and processing the data by hand can be time consuming.weathercan
automatically downloads and formats data from multiple stations and over large date ranges.https://github.com/steffilazerte/weathercan
data retrieval:
weathercan
downloads multiple data sets at a time from the Environment and Climate Change Canada website.data munging:
weathercan
tidies and combines the downloaded data into a format ready for analysis in R.weathercan
also provides tools for conducting linear interpolation for adding weather data to other data sets which might not perfectly align with the timing of weather observations.Any one who needs access to Canadian weather data: Biologists, Meteorologists, Geographers, Hydrologists, Climatologists, Students, Teachers, etc.
yours differ or meet our criteria for best-in-category?
rclimateca
is a package available on CRAN that is similar toweathercan
.Both packages concern the downloading of Environment and Climate Change Canada weather data into an easy format for use in R.
The largest difference between the two packages is that
weathercan
includes functions for interpolating weather data and directly integrating it into other data sources. Another difference is thatweathercan
actively seeks to apply tidy data principles in R and integrates well with the tidyverse including using tibbles and nested listcols. Theweathercan
package also includes a website and tutorials/vignettes. We would also argue thatweathercan
is slightly more user-friendly.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
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings:Status: OK
R CMD check results
0 errors | 0 warnings | 0 notes
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:
Best:
Alternative:
https://github.com/bpbond (could be potential user for this package or packages like this)
https://github.com/aappling-usgs (could be potential user for this package or packages like this)
Extra: goodpractice::gp() output:
── GP weathercan ─────────────────────────────────────────────
It is good practice to
✖ write unit tests for all functions, and all package code in general. 82% of code lines are covered by test cases.
✖ avoid long code lines, it is bad for readability. Also, many people prefer editor windows that are about 80
characters wide. Try make your lines shorter than 80 characters
✖ avoid sapply(), it is not type safe. It might return a vector, or a list, depending on the input data. Consider using
vapply() instead.
✖ fix this R CMD check NOTE: Note: found 26 marked UTF-8 strings
Comment:
This note doesn't pop up when I run
devtools::check()
on my linux machine, so I'm not really sure what's going on. I wouldn't mind some assistance in determining:a) whether it matters
b) if it does, how to fix it in a cross platform manner
The text was updated successfully, but these errors were encountered: