Skip to content
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

Closed
14 of 15 tasks
steffilazerte opened this issue Nov 24, 2017 · 41 comments
Closed
14 of 15 tasks

weathercan #160

steffilazerte opened this issue Nov 24, 2017 · 41 comments

Comments

@steffilazerte
Copy link
Member

Summary

  • What does this package do? (explain in 50 words or less):

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.

  • Paste the full DESCRIPTION file inside a code block below:
Package: weathercan
Type: Package
Title: Download Weather Data from the Environment and Climate Change Canada Website
Version: 0.2.2.9000
Authors@R: c(
     person("Steffi", "LaZerte", email = "steffi@steffi.ca", role = c("aut","cre")),
     person("Sam", "Albers", email = "sam.albers@gmail.com", role = c("ctb")))
Description: Provides means for downloading historical weather data from 
    the Environment and Climate Change Canada website 
    (<http://climate.weather.gc.ca/historical_data/search_historic_data_e.html>). 
    Data can be downloaded from multiple stations and over large date ranges 
    and automatically processed into a single dataset. Tools are also provided 
    to identify stations either by name or proximity to a location.
License: GPL-3
BugReports: https://github.com/steffilazerte/weathercan/issues
LazyData: TRUE
URL: https://github.com/steffilazerte/weathercan
Depends:
    R (>= 3.3.3)
Imports:
    dplyr (>= 0.7.0),
    httr (>= 1.1.0),
    lubridate (>= 1.7.1),
    magrittr (>= 1.5),
    methods (>= 3.2.2),
    sp (>= 1.2-3),
    stringi (>= 1.1.2),
    tibble (>= 1.2),
    tidyr (>= 0.4.1),
    xml2 (>= 0.1.2)
RoxygenNote: 6.0.1
Suggests:
    devtools,
    ggplot2,
    knitr,
    rmarkdown,
    stringr,
    testthat
VignetteBuilder: knitr
  • URL for the package (the development repository, not a stylized html page):

https://github.com/steffilazerte/weathercan

  • 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.):

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.

  • Who is the target audience?

Any one who needs access to Canadian weather data: Biologists, Meteorologists, Geographers, Hydrologists, Climatologists, Students, Teachers, etc.

rclimateca is a package available on CRAN that is similar to weathercan.

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 that weathercan actively seeks to apply tidy data principles in R and integrates well with the tidyverse including using tibbles and nested listcols. The weathercan package also includes a website and tutorials/vignettes. We would also argue that weathercan is slightly more user-friendly.

Requirements

Confirm each of the following by checking the box. This package:

  • does not violate the Terms of Service of any service it interacts with.
  • has a CRAN and OSI accepted license.
  • contains a README with instructions for installing the development version.
  • includes documentation with examples for all functions.
  • contains a vignette with examples of its essential functions and uses.
  • has a test suite.
  • has continuous integration, including reporting of test coverage, using services such as Travis CI, Coeveralls and/or CodeCov.
  • I agree to abide by ROpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

Publication options

  • Do you intend for this package to go on CRAN?
  • Do you wish to automatically submit to the Journal of Open Source Software? If so:
    • The package has an obvious research application according to JOSS's definition.
    • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
    • The package is deposited in a long-term repository with the DOI:
    • (Do not submit your package separately to JOSS)

Detail

  • Does R CMD check (or devtools::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:

── 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.

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 92 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

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

@sckott sckott added the package label Nov 24, 2017
@sckott
Copy link
Contributor

sckott commented Nov 24, 2017

👋 @steffilazerte - thanks for your submission! We'll discuss fit for rOpenSci and get back to you soon.

@sckott
Copy link
Contributor

sckott commented Nov 24, 2017

ah woops, sorry. forgot we already covered this in presubmission thing. Will assign an editor soon

@sckott
Copy link
Contributor

sckott commented Nov 28, 2017

Editor checks:

  • Fit: The package meets criteria for fit and overlap
  • Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
  • License: The package has a CRAN or OSI accepted license
  • Repository: The repository link resolves correctly
  • Archive (JOSS only, may be post-review): The repository DOI resolves correctly
  • Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Thanks for your submission @steffilazerte !!

Goodpractice output for you and reviewers to consider:

It is good practice towrite 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 linesavoid 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 linesavoid 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 linesfix this R CMD check NOTE: Note: found 26 marked UTF-8
    strings

Seeking reviewers now.


Reviewers: @joethorley @softloud
Due date: 2017-12-20

@sckott
Copy link
Contributor

sckott commented Nov 29, 2017

Reviewers assigned. They are: @joethorley @softloud
Due date is 2017-12-20

@joethorley
Copy link

Package Review

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work.

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • Vignette(s) demonstrating major functionality that runs successfully locally
  • Function Documentation: for all exported functions in R help
  • Examples for all exported functions in R Help that run successfully locally
  • Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).
For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • A short summary describing the high-level functionality of the software
  • Authors: A list of authors with their affiliations
  • A statement of need clearly stating problems the software is designed to solve and its target audience.
  • References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
  • Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

  • The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 5


Review Comments

This is a useful package to locate Environment Canada weather stations,
and download and interpolate the data.

A statement of need

The README definitely requires a brief discussion of how the current package differs from rclimateca and CHCN.

Examples

An example is lacking for stations_all() - this is minor but technically in violation of the documentation policy.

README

The demonstration of the use of dplyr::select is unnecessary in the README.

Vignettes

Packaging Guidelines

The function naming should follow the suggested object_verb() naming scheme ie weather() should become something like weather_download(), stations_all() to stations_download(), and add_weather() to weather_interpolate() etc.

The README lacks citation information.

The two most recent NEWS.md headings lack dates.

DESCRIPTION

The DESCRIPTION file should include the Date of the current version.

Coding Style

The 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.
Only use return() for early returns.

@steffilazerte
Copy link
Member Author

Thanks @joethorley, this all looks pretty straightforward, thanks for your speedy review! I'll start looking into these changes.

@sckott
Copy link
Contributor

sckott commented Dec 15, 2017

@softloud please get your review in by next wednesday, thanks!

@softloud
Copy link

Thanks for the prompt. I am impressed by the efficiency of rOpenSci! Hoping to get it done this weekend.

@softloud
Copy link

softloud commented Dec 19, 2017

Package Review

  • As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

  • A statement of need clearly stating problems the software is designed to solve and its target audience in README
  • Installation instructions: for the development version of package and any non-standard dependencies in README
  • [ ]Vignette(s) demonstrating major functionality that runs successfully locally
  • [x]Function Documentation: for all exported functions in R help
  • [ ]Examples for all exported functions in R Help that run successfully locally
  • [x]Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).
For packages co-submitting to JOSS

The package contains a paper.md matching JOSS's requirements with:

  • [x]A short summary describing the high-level functionality of the software
  • [x]Authors: A list of authors with their affiliations
  • [x]A statement of need clearly stating problems the software is designed to solve and its target audience.
  • [x]References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

  • [x]Installation: Installation succeeds as documented.
  • [x]Functionality: Any functional claims of the software been confirmed.
  • [x]Performance: Any performance claims of the software been confirmed.
  • [x]Automated tests: Unit tests cover essential functions of the package
    and a reasonable range of inputs and conditions. All tests pass on the local machine.
  • [ ]Packaging guidelines: The package conforms to the ROpenSci packaging guide

Final approval (post-review)

  • [ ]The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing:


Review Comments

I appreciate the opportunity to review a package like this; I learnt a lot from its well-designed structure. I like the idea of a tidy-friendly glossary dataframe with accompanying vignette.

Vignettes

Now 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.

> library(weathercan)
> vignette("usage") # No output other than warning.
Warning message: 
vignette ‘usage’ not found 
> vignette("broom") # The broom vignette opens in the Help pane. 

Examples

As @joethorley noted, there are one or two functions that are missing an example. When I went back through I think it was just stations_all that needs a brief example for the function documentation to conform to the rOpenSci package guide.

Testing

There was one failure when running tests. This looks very minor, if it is anything to worry about. The tests also appear comprehensive.

==> devtools::test()

Loading weathercan
Loading required package: testthat
Testing weathercan
✔ | OK F W S | Context
✖ | 87 1     | Raw interpolation (time) [1.0 s]
───────────────────────────────────────────────────────────────────────
test_interpolate.R:78: failure: approx_na_rm (time) replaces gaps with NAs
sum(is.na(a$y)) not equal to 195.
1/1 mismatches
[1] 0 - 195 == -195
───────────────────────────────────────────────────────────────────────

Other tests

I ran various other ad hoc tests, including the goodpractice package, but didn't find anything other than the one error thrown by testthat to report. So, barring a few minor tweaks, perhaps, the weathercan package seems ready for submission to me.

@sckott
Copy link
Contributor

sckott commented Dec 19, 2017

thanks for your review @softloud !

@steffilazerte both reviews are in, continue discussion here ...

@joethorley
Copy link

FWIW I built the vignettes successfully on OSX High Sierra using install.packages("~/Code/weathercan/weathercan_0.2.2.9000.tar.gz", repos = NULL, type = "source")

@steffilazerte
Copy link
Member Author

Great thanks to both of you. I'm not sure what's going on with that test but I'll take a look.
In general, I've been incorporating changes based on your suggestions to the developmental branch and will push to the master branch sometime after Christmas!

@steffilazerte
Copy link
Member Author

steffilazerte commented Jan 3, 2018

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 @joethorley

A statement of need

  • The README definitely requires a brief discussion of how the current package differs from rclimateca and CHCN.
    Response: Added in ropensci/weathercan@648177d

Examples

  • An example is lacking for stations_all() - this is minor but technically in violation of the documentation policy.
    Response: Added in ropensci/weathercan@1645a34

README

Packaging Guidelines

  • The function naming should follow the suggested object_verb() naming scheme ie weather() should become something like weather_download(), stations_all() to stations_download(), and add_weather() to weather_interpolate() etc.
    Response: I've changed 5 function names (4 exported, 1 internal): weather_dl (from weather), weather_interp (from add_weather), stations_dl (from stations_all), tz_calc (from get_tz), and weather_raw (from weather_dl, the internal function) in ropensci/weathercan@8493e35

  • The README lacks citation information.
    Response: Added in ropensci/weathercan@84c640b

  • The two most recent NEWS.md headings lack dates.
    Response: Added in ropensci/weathercan@0d7dfd7

DESCRIPTION

Coding Style

  • The 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.
    Response: I'll definitely bear this in mind for the future. If you think it really necessary, I can redo it here, but I'd rather not.

  • Only use return() for early returns.
    Response: Fixed in ropensci/weathercan@839cccc

Response to @softloud

  • Trouble accessing vignettes
    Response: After looking into this, I realized that to install vignettes, one should use devtools::install_github("steffilazerte/weathercan", build_vignettes = TRUE). Vignettes can be listed with vignette(package = "weathercan"). I've added these additional details to the README to help future users (let me know if this doesn't solve your problem!) Fixed in ropensci/weathercan@6439118

  • Missing example
    Response: Added (as above) in ropensci/weathercan@1645a34

  • Failed test
    Response: I'm still not sure why this test failed on your system. Occasionally I have run into local problems with timezones (the test is for determining the timezone from lat/lon by dialing into Google), and sometimes I find a test will fail randomly on AppVeyor or TravisCi but restarting the test build always seems to resolve the issue. I think it may have something to do with calling into Google. If you haven't run into any other problems, I'll leave it for now unless it starts to cause some major issues.

Extra Changes

@sckott
Copy link
Contributor

sckott commented Jan 3, 2018

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

@joethorley
Copy link

@sckott I am happy with the authors response - I have no further comments

@sckott
Copy link
Contributor

sckott commented Jan 4, 2018

thanks @joethorley

@sckott sckott mentioned this issue Jan 5, 2018
3 tasks
@sckott
Copy link
Contributor

sckott commented Jan 24, 2018

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

@sckott
Copy link
Contributor

sckott commented Jan 24, 2018

Just a few things I noticed:

  • remove the Date from the DESCRIPTION file - i realize a rewiewer suggested that, but it should be removed - it gets added to the pkg when it's built on the CRAN servers
  • goodpractice highlights that you have a lot of lines of code that are longer than 80 characters. that's not a make or break thing, but I'd strongly recommend shortenin lines to 80 width
  • gp also highlights use of sapply(). Strongly recommend avoiding sapply

of the above, can you take care of the 1st and 3rd before we move on?

@steffilazerte
Copy link
Member Author

Absolutely. I'll get back to you soon (may take a day or two, depending when I get a free moment). Thanks!

@steffilazerte
Copy link
Member Author

Okay, I've removed the Date from the DESCRIPTION file, shortened all the too long lines and switched vapply() for sapply(). The master branch has all changes!

@sckott
Copy link
Contributor

sckott commented Jan 30, 2018

Great, thanks much @steffilazerte taking a peek

@sckott
Copy link
Contributor

sckott commented Jan 30, 2018

Approved! Thanks again for your submission @steffilazerte

  • Please transfer the package to ropensci- I've invited you to a team within our ropensci organization. After you accept you should be able to move it.
  • Make sure to
    • add rOpenSci footer to README
      [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)
    • Change any needed links, such those for CI badges
    • Travis CI should update to the new location automatically - you may have to update other CI systems manually
  • You already have dotgithub files (awesome!)
  • We're starting to roll out software metadata files to all ropensci pkgs via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your pkg, after installing the pkg - should be easy as running codemetar::write_codemeta() in the root of your pkg

@sckott
Copy link
Contributor

sckott commented Jan 30, 2018

@steffilazerte closing issue now b/c approval, but can continue any discussion here still

@sckott sckott closed this as completed Jan 30, 2018
@steffilazerte
Copy link
Member Author

Okay sounds good, it asks me which teams should have access, I assume only two teams: Leadership and weathercan?

  • I'll update the links, add the footer and add metadata files after I transfer it (easiest way to make sure they work!)
  • Definitely interested in blog post (my collaborator, @boshek, had an idea of showcasing weathercan in a spatial framework.)
  • Where do we go with the JOSS manuscript?

@sckott
Copy link
Contributor

sckott commented Jan 31, 2018

I assume only two teams

yep, thanks

Definitely interested in blog post

Great, @stefaniebutland will get in touch with you

Where do we go with the JOSS manuscript?

@stefaniebutland
Copy link
Member

@steffilazerte

Definitely interested in blog post

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.

@steffilazerte
Copy link
Member Author

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!

@steffilazerte
Copy link
Member Author

@sckott

  • weathercan was transferred to ropensci
  • I've updated all the links to ropensci (including the website and vignettes)
  • Added ropensci footer
  • added codemeta.json

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?

@sckott
Copy link
Contributor

sckott commented Feb 1, 2018

🚀 thanks

zenodo integration turned on and you and sam now have admin rights on repo

@stefaniebutland
Copy link
Member

Hi @steffilazerte. Further thoughts on a date to submit a draft blog post? draft by Tues Feb 27th for publication the following week?

@steffilazerte
Copy link
Member Author

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?

@stefaniebutland
Copy link
Member

No problem - was not even a delay. Name your preferred date to submit a draft and we'll work with that.

@steffilazerte
Copy link
Member Author

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!

@steffilazerte
Copy link
Member Author

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 weathercan is pretty simple, I've been playing around with the idea of highlighting how to integrate data downloaded through weathercan into other datasets (as that's how most people would use it), be it spatial (e.g. with sf and then perhaps mapview or leaflet) or biological (e.g. interpolating the data to integrate it with bird activity data from my feedr package).

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!

@stefaniebutland
Copy link
Member

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?

@boshek
Copy link

boshek commented Feb 16, 2018

@stefaniebutland @steffilazerte Happy to read!

@steffilazerte
Copy link
Member Author

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.
Thanks!

@stefaniebutland
Copy link
Member

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/

@stefaniebutland
Copy link
Member

@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.

@steffilazerte
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants