riem package -- access to METAR through Iowa Environment Mesonet about software-review HOT 20 CLOSED

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

from software-review.

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.

from software-review.

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

from software-review.

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.

from software-review.

Thanks for your submission! Seeking reviewers now

from software-review.

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

from software-review.

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

from software-review.

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

from software-review.

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.

from software-review.

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

from software-review.

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

from software-review.

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

from software-review.

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.

from software-review.

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

from software-review.

dont need to ruildignore https://github.com/masalmon/riem/blob/master/.Rbuildignore#L5

from software-review.

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

from software-review.

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.

from software-review.

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

from software-review.

Okay, sounds good.

from software-review.

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?

from software-review.