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

riem package -- access to METAR through Iowa Environment Mesonet #39

Closed
maelle opened this Issue Apr 25, 2016 · 20 comments

Comments

Projects
None yet
5 participants
@maelle
Copy link
Member

maelle commented Apr 25, 2016

Package: riem
Type: Package
Title: Accesses Data from the Iowa Environment Mesonet
Version: 0.1.0
Authors@R: person("Maëlle", "Salmon", email = "maelle.salmon@yahoo.se", role =
    c("aut", "cre"))
Description: Allows to get weather data from ASOS stations (airports) in the
    whole world thanks to the Iowa Environment Mesonet website.
License: GPL (>= 2)
LazyData: TRUE
Imports:
    httr,
    lubridate,
    dplyr,
    jsonlite,
    readr,
    lazyeval
RoxygenNote: 5.0.1
Suggests: testthat,
    knitr,
    rmarkdown
VignetteBuilder: knitr


    1. URL for the package (the development repository, not a stylized html page)
      https://github.com/masalmon/riem
    1. What data source(s) does it work with (if applicable)?
      Iowa Environment Mesonet https://mesonet.agron.iastate.edu/request/download.phtml?network=IN__ASOS
    1. Who is the target audience?
      Anyone needing weather data
    1. Are there other R packages that accomplish the same thing? If so, what is different about yours?
      The R package wunderground allows to access weather data including ASOS stations but the API only allows a linited number of calls per day/minute.
    1. Check the box next to each policy below, confirming that you agree. These are mandatory.
    • [x ] This package does not violate the Terms of Service of any service it interacts with.
    • [ x] The repository has continuous integration with Travis CI and/or another service
    • [ x] The package contains a vignette
    • [ x] The package contains a reasonably complete README with devtools install instructions
    • [ x] The package contains unit tests
    • [ x] The package only exports functions to the NAMESPACE that are intended for end users
    1. Do you agree to follow the rOpenSci packaging guidelines? These aren't mandatory, but we strongly suggest you follow them. If you disagree with anything, please explain.
    • Are there any package dependencies not on CRAN?
    • [x ] Do you intend for this package to go on CRAN?
    • [ x] Does the package have a CRAN accepted license?
    • Did devtools::check() produce any errors or warnings? If so paste them below.
    1. Please add explanations below for any exceptions to the above:
    1. If this is a resubmission following rejection, please explain the change in circumstances.
@sckott

This comment has been minimized.

Copy link
Member

sckott commented Apr 26, 2016

Thanks for your submission! Seeking reviewers now

@sckott

This comment has been minimized.

Copy link
Member

sckott commented Apr 27, 2016

Reviewers: @geanders
Due date: 2016-05-18

@sckott

This comment has been minimized.

Copy link
Member

sckott commented May 13, 2016

@geanders
Due date: 2016-05-18 - hey there, it's been 16 days, please get your review in by May 18, thanks 😺 (ropensci-bot)

@geanders

This comment has been minimized.

Copy link
Collaborator

geanders commented May 17, 2016

Overall, the package is short and sweet, with 3 cleanly coded and well-named functions. I would certainly use this package in my own research and recommend it to others. The documentation, both on the README page for the GitHub repository and in the vignette, gives an appropriate amount of details on the data that’s being pulled and the source from which it came (although, see a few minor notes below about a few points in the data description to clarify) and includes a prominently-placed link to the source’s website for users who want more information on the original data. I have only a few minor suggestions for improvements.

  • All examples in README worked easily in a clean R session. It was clear what each function was doing.
  • Functions are named in a way that is clear and easy to remember.
  • What time zone is the time stamp in?
  • Defining the measures (units, etc.) in the README is very helpful. However, it looks like this is directly copied and pasted from the IEM website. That’s fine, but make sure it’s clear that it was copied from there. Also, add in the units for relative humidity (same comment for the riem_measures help file— from looking at the output, I’m pretty sure it’s in %). Finally, could you give a link or some other help to users for how to decrypt the weather codes (one of the variables returned)?
  • In the help files for the function, it would be helpful to users if you included a link to the IEM site, if they need more information about the original data.
  • Include a note in the help file for riem_stations that users can see a map with all stations available for a network at the IEM website. For some users, that might be an easier way to search for the codes to use with riem_measures.
  • For the help file for riem_stations, make sure that it’s clear that you can only put in a single network at a time. Some users might try to put in a vector of several networks, if they want to pull for several countries at a time. Currently, it’s not completely clear from the documentation that you can’t do this, and the error message if you try would be cryptic to many users, I think.
  • There’s nothing wrong with the use of strsplit to split up the dates in riem_measures, but if lubridate is already a dependency, why not just use functions from that to process the date? I would think the ymd function would give a little more leniency in how users input the date (for example, could probably process “2000-01-01” and “2000/01/01”). Then you could use year, month and mday to pull out the elements you want.
  • Throughout the code, I would recommend using the :: syntax when you can for functions that aren’t from base R rather than using @importFrom.
  • The tests included with the package will be useful for quickly identifying if IEM has changed its API in a way that affects this package. That seems like a great safeguard for maintaining the package.
  • Add minimum version numbers for the dependencies listed in DESCRIPTION
  • You should definitely contact the group at IEM and let them know about this package! They have a link up to a Python script already— I think they might be thrilled to point R users to a package that helps them pull in the data.
@maelle

This comment has been minimized.

Copy link
Member Author

maelle commented May 17, 2016

Thanks a lot, @geanders! 😀 I will improve the pkg this week!

@sckott

This comment has been minimized.

Copy link
Member

sckott commented May 18, 2016

@geanders
Due date: 2016-05-18 - hey there, it's been 21 days, please get your review in soon, thanks 😺 (ropensci-bot)

@maelle

This comment has been minimized.

Copy link
Member Author

maelle commented May 18, 2016

I've now taken Brooke's feedback into account, which was a pleasure, thanks a lot again for your useful feedback and your enthusiasm! I have added you as a reviewer in the DESCRIPTION file.

  • I added two links before describing the output in the README and the vignette.
  • I added the timezone (UTC) of the time stamp.
  • I added the % unit for relative humidity and a link to a manual for the present weather codes (this could be useful indeed!).
  • I added a link to the website in the measures help file and also in the stations help file with a remark about the map.
  • I added "A single" regarding network for riem_stations.
  • I switched from strsplit to lubridate functions, this is a really good idea!
  • I got rid of most importFrom except for "%>%"
  • I added versions for imported packages
  • I had contacted the person responsible for the website before writing the package, but I'll write to him again once the package is in ropenscilabs!
  • I really hope I haven't missed anything but otherwise I'll be happy to correct any mistake!

@geanders & @sckott I'll now wait for further steps.

@sckott

This comment has been minimized.

Copy link
Member

sckott commented May 19, 2016

@geanders sorry about the 2nd bot ping #39 (comment) - still working out bugs in the system

@sckott

This comment has been minimized.

Copy link
Member

sckott commented May 23, 2016

@masalmon I'll have a quick look today and get back to you

@sckott

This comment has been minimized.

Copy link
Member

sckott commented May 25, 2016

Looks great. Just a few things before accepting::

.Rbuildignore this

checking DESCRIPTION meta-information
N  checking top-level files
   Non-standard file/directory found at top level:README.Rmd

Are these warnings a problem?

Testing riem
measures: ..........................W.W.W.W..
networks: ...
stations: ......

Warnings -----------------------------------------------------------------------
1. riem_measures checks dates (@test-measures.R#41) - All formats failed to parse. No formats found.

2. riem_measures checks dates (@test-measures.R#43) - All formats failed to parse. No formats found.

3. riem_measures checks dates (@test-measures.R#45) - All formats failed to parse. No formats found.

4. riem_measures checks dates (@test-measures.R#47) - All formats failed to parse. No formats found.

Examples took a while to run

^@checking examples
   Examples with CPU or elapsed time > 5s
                  user system elapsed
   riem_measures 0.591   0.12  17.429

but it could just be my slow hotel internet - if you don't get a time warning, then nevermind

@maelle

This comment has been minimized.

Copy link
Member Author

maelle commented May 25, 2016

Danke!

  • I have added the README files to .Rbuildignore.
  • Yes the warnings are ok. They come from a test where I knowingly give wrongly formatted dates as input. I give an error message but there's the lubridate warning too. Would it be better to add a suppressWarnings? Somehow I didn't feel at ease adding a suppressWarnings here.
  • Oh, I hadn't noticed the examples running time... And it was good to remember to put them in \dontrun{} for CRAN so now there's no problem.

Moreover,

  • I have added the purl and eval options in the vignette as preparation for a CRAN submission.
@maelle

This comment has been minimized.

Copy link
Member Author

maelle commented May 25, 2016

@akrherz thanks again for the help about the IEM website!

@sckott

This comment has been minimized.

Copy link
Member

sckott commented May 25, 2016

@sckott

This comment has been minimized.

Copy link
Member

sckott commented May 25, 2016

you may want to have some examples not in \dontrun - cran folks sometimes complain about lack of egs not in dontrun, but mostly they dont complain

@maelle

This comment has been minimized.

Copy link
Member Author

maelle commented May 25, 2016

Would it be ok to have examples using a website in dontrun? I'd rather let them in dontrun... Well but CRAN folks don't know I have CI running the examples on a regular basis... I'm undecided.

@maelle

This comment has been minimized.

Copy link
Member Author

maelle commented May 25, 2016

In opencage all examples are in \dontrun and they didn't complain. So I might try with dontrun for a first submission!

@sckott

This comment has been minimized.

Copy link
Member

sckott commented May 25, 2016

Okay, sounds good.

@sckott sckott added the 6/approved label May 25, 2016

@sckott

This comment has been minimized.

Copy link
Member

sckott commented May 25, 2016

@masalmon accepted, transfer to ropenscilabs when you get a chance :)

@maelle maelle closed this May 25, 2016

@mpadge mpadge referenced this issue Jun 24, 2017

Closed

bomrang #121

14 of 14 tasks complete
@gregfortress

This comment has been minimized.

Copy link

gregfortress commented Dec 20, 2018

Hi, I'm no R expert (or coding, really), but my database runs this package at 6 am in the morning to grab yesterday's wind and temp. But it can only get hours 1-20. 21-24 never come in. Do the rest of the hours of the previous day get published after 6am EST? How do I get those last few hours?

@noamross

This comment has been minimized.

Copy link
Collaborator

noamross commented Dec 20, 2018

Hi @gregfortress, please file your issue over at the package repository: https://github.com/ropensci/riem/issues . This is a closed thread from a previous package review; that is the appropriate place for ongoing questions/issues.

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