Comments (20)
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 withriem_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 inriem_measures
, but if lubridate is already a dependency, why not just use functions from that to process the date? I would think theymd
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 useyear
,month
andmday
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
tolubridate
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.
Related Issues (20)
- gigs: Assess Fetal, Newborn, and Child Growth with International Standards HOT 8
- dendroNetwork HOT 50
- eDNAjoint: an R package for interpreting paired environmental DNA and traditional surveys HOT 16
- Capybara HOT 9
- Presubmission inquiry: `chopin` HOT 2
- Presubmission enquiry for cocoon: Extract, Format, and Print Statistical Output HOT 3
- QuadratiK: A Collection of Methods Using Kernel-Based Quadratic Distances for Statistical Inference and Clustering HOT 52
- osmapiR: an implementation of OpenStreetMap API v0.6 for R HOT 89
- ## Reply to package review by @njtierney HOT 3
- cancerprof: API Client for extracting data from State Cancer Profiles HOT 3
- rsi: Efficiently Retrieve and Process Satellite Imagery HOT 44
- cancerprof: API Client for extracting data from State Cancer Profiles HOT 41
- `chopin`: Computation for Climate and Health research On Parallelized INfrastructure HOT 54
- rpigpior HOT 13
- phangorn HOT 2
- eDNAjoint: R package for interpreting paired environmental DNA and traditional surveys HOT 44
- Presubmission Inquiry of the `cryptoQuotes`-package HOT 6
- Capybara HOT 22
- QuickJSR - Portable, lightweight JavaScript engine for R
- feowR: Download shapefiles of Freshwater Ecoregions of the World (FEOW) HOT 12
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from software-review.