ropensci/software-review

nasapower

adamhsparks opened this issue ยท 74 comments

adamhsparks commented

Summary

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

nasapower automates downloading NASA-POWER agroclimatology data in your R
session as a tidy data frame for analysis, modelling or other purposes by interfacing with the NASA POWER API. POWER (Prediction Of Worldwide Energy Resource) data are freely available for download through a web interface at a resolution of 0.5หš longitude by 0.5หš latitude.

  • Paste the full DESCRIPTION file inside a code block below:
Package: nasapower
Type: Package
Title: NASA-POWER Data from R
Version: 1.0.0.9000
Authors@R: person(given = "Adam H.", family = "Sparks",
    role = c("aut", "cre"), email = "adamhsparks@gmail.com",
    comment = c(ORCID = "0000-0002-0061-8359"))
URL: https://github.com/adamhsparks/nasapower
BugReports: https://github.com/adamhsparks/nasapower/issues
Description: Download NASA-POWER global meteorology and surface solar energy
    climatology data and create a tidy data frame for 141 parameters.  nasapower
    provides an interface to the NASA - POWER API.  POWER (Prediction Of
    Worldwide Energy Resource) data are freely available for download with a
     resolution of 0.5 by 0.5 arc degree longitude and latitude and are funded
    through the NASA Earth Science Directorate Applied Science Program. For more
    on the data themselves, please see <https://power.larc.nasa.gov/>.
Depends:
    R (>= 3.2.0)
License: MIT + file LICENSE
Imports:
    crul,
    lubridate,
    jsonlite,
    readr,
    tibble
RoxygenNote: 6.0.1
Roxygen: list(markdown = TRUE)
Encoding: UTF-8
NeedsCompilation: no
Repository: CRAN
LazyData: TRUE
ByteCompile: TRUE
Suggests:
    APSIM,
    covr,
    dplyr,
    ggplot2,
    knitr,
    raster,
    rmarkdown,
    testthat,
    vcr
VignetteBuilder: knitr
X-schema.org-applicationCategory: Tools
X-schema.org-keywords: NASA, meteorological-data, weather, global, weather, weather-data, meteorology, NASA-POWER, agroclimatology, earth-science, data-access
  • URL for the package (the development repository, not a stylized html page):

https://github.com/adamhsparks/nasapower

  • 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 and extraction, nasapower automates downloading and parsing
NASA - POWER data into a useful format in R using the POWER API.

  • Who is the target audience?

Researchers who use NASA - POWER for modelling or other purposes though there may
be others that I'm not familiar with. This is the audience I know best since
I'm part of it and use these data for my own work.

Since the POWER API is brand new, no. This is the first R package to utilise it's capabilities.

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:

  • Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:

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

I've picked on everyone I can think of previously for other submissions.

adamhsparks commented

I was looking at the POWER website. The most, but not all, of the other POWER datasets have similar interfaces. It should be pretty simple add some of the Renewable Energy and Sustainable Buildings data to the package as well. I've just never used them for my work. It would be good if these were added, that someone with some knowledge of them could provide some assistance on what's useful.

Happy to get feedback on that.

sckott commented

Thanks @adamhsparks - Editors are discussing ...

  • ๐Ÿ‘1
sckott commented

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

Goodpractice output:

โ”€โ”€ 0 errors โœ” | 0 warnings โœ” | 0 notes โœ”
โ™ฅ Aha! Astounding package! Keep up the best work!

Seeking reviewers now.

Reviewers: @alisonboyer @UniversalTourist
Due date: 2017-12-06

sckott commented

Reviewers are @alisonboyer and @UniversalTourist - due date is 2017-12-06

  • ๐ŸŽ‰2
UniversalTourist commented

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

  • 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
  • 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: It took almost, totally, 4-5 hours to check everything. During that time I studied what is NASA-POWER, agroclimatology and read rOpenSci guidelines, learned about Travis-CI and running tests.

Review Comments

This is the first time I heard about NASA-POWER and it is a great data source for Earth scientists. These package provides a data frame output and it will make the data preparation process more easier than copy-pasting.
(I have tested functions for region of my country and already sent the package's Github link for my colleges :) )

  • Installation:
    With MacOS:
    I used macOS Sierra, Verison 10.12 and I was able to install the package from Github without any problem by devtools.

  • Code Overall:
    These package has two functions to get data for a cell or for a region. Both functions has nice structered, easy to follow and also nice explained with comments, I enjoyed reviewing this part especially. (I also learned about how httr works by running these codes step by step.)

  • rOpenSci criteria:
    Package name is really nice. Function and variable naming is proper. Licence and Code of Conduct are available.

  • Readme:
    Readme.rmd is clear and easy to follow steps for installations.

  • Vignette:
    There are more details and examples about other function. Also the examples provide more information and help to illustrate the importance of the package.
    In the introduction part there is only one mistake, the "which will" words are written for two times: "both of which will which will download".
    I notified author about plots on vignette which seems different when I run on my system, and problem was resolved with a commit fastly when I contacted with the author.

  • Errors and console messages:
    They are all clear.

  • R CMD check:
    R CMD check passes without any warnings or any errors.

  • 0 errors | 0 warnings | 0 notes
    :)

  • Other comments
    NASA-POWER provides data at a resolution of 1หš longitude by 1หš latitude, which is good enough most of the time. Maybe you can also write about other data sources which provide higher resolutions in the package readme file for the people who come to this package repo for higher resolution.

p.s This is my first time at reviewing a package and I hope I can be helpful :)

  • ๐Ÿ‘2
sckott commented

thx very much for your review @UniversalTourist

adamhsparks commented

Thank you @UniversalTourist

I have fixed the vignette text you noted where it said "which will which will", ropensci/nasapower@6dd0d9c.

I have also added a link to other gridded climatology data sets. As far as I know, nothing really aligns with what NASA POWER offers at a higher resolution, but there are other gridded climatology data sets out there, ropensci/nasapower@8b8a11a

UniversalTourist commented

Thank you very much!

  • ๐Ÿ‘1
sckott commented

@alisonboyer Your review was due a few days ago. Can you get it in soon?

alisonboyer commented
alisonboyer commented

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

Functionality

  • Installation: Installation succeeds as documented.
  • Functionality: Any functional claims of the software been confirmed.
  • Performance: Any performance claims of the software been confirmed.
    Comment: How large of a region can you request using this service? Some notes about the expected size of the data frame returned would be helpful. For example, a 2 x 4 degree region with 3 variables and all available time steps was ~ 2MB. If I request the entire globe, what will happen?
  • 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.
    Comment: I do not know how to run unit tests. My local tests of the basic functionality all ran flawlessly.
  • Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing: 2.5

Review Comments:

Providing a recommended citation for the data is very nice. Does the data have a DOI?
The link to other climate data provided by NOAA ESRL is helpful.

  • ๐ŸŽ‰1
adamhsparks commented

Thank you, @alisonboyer.

I didn't have a statement about performance because it will vary by the data requested and Internet connection. However, I ran an example this morning and added a statement to the vignette in this commit, ropensci/nasapower@3bab17e about what some possible realistic times and data frame sizes could be for this. See starting at line 114 in the nasapower.Rmd vignette for the statement.

AFAIK there is no DOI for the POWER data. As you noted, I've included their statement about how they wish to be acknowledged and the link to https://power.larc.nasa.gov/documents/Agroclimatology_Methodology.pdf is included in the documentation in several locations as well.

adamhsparks commented

For both @UniversalTourist and @alisonboyer, I've also expanded the documentation in the vignette to provide an example of how to turn the data frame in to a raster::brick spatial object for users that need it to be a spatial object not a normal data frame.

https://adamhsparks.github.io/nasapower/articles/nasapower.html#creating-spatial-objects

adamhsparks commented

This afternoon the POWER webpage stopped serving data. I've added a check now that pings the server first, five times, to see if it's responding, before proceeding with the request.

If the reviewers could check this as it was added after both reviews?

ropensci/nasapower@94e2440

UniversalTourist commented

Hey Adam!
I will check bot spatial example and this ping issue later today! And let you know.

Cheers.

  • ๐Ÿ‘1
adamhsparks commented

@UniversalTourist, please wait. Now that it's responding again, it's clear that my fix didn't work. I need to find a new way.

adamhsparks commented

OK, fixed now with a new better way of doing checking whether the NASA - POWER site is serving data or not.

Sorry about that.

UniversalTourist commented

Everything seems OK! Spatial objects example will really make this package more attractive! ๐Ÿ’ฏ And I love the sticker! ๐Ÿ˜„

  • ๐ŸŽ‰1
adamhsparks commented

Thank you Hazel!

adamhsparks commented

@UniversalTourist and @alisonboyer, I've added a use-case vignette now, https://adamhsparks.github.io/nasapower/articles/use-case.html. And I've included further validation of user-entered lon/lat values for both functions, ropensci/nasapower@df48fd8. I hope that I've covered off on all the requested changes (and then some).

@sckott, I've started drafting a JOSS paper. I was waiting to see if there were any major changes requested by the reviewers before writing it.

sckott commented

@adamhsparks If okay with you, i want to take a quick look at the latest version to see if there's anything to comment on ...

adamhsparks commented
sckott commented

some thoughts:

for both get_cell and get_region

  • unless you have some edge case stuff, would it not be easier to construct a named list of query params to pass to GET?
  • seems smart to break apart the GET and content calls on separate lines, w/ a stop_for_status() or similar in between them so users get nice error messages when a server or client error happens
  • I think there's no check on the vars param internally, so e.g. ,
get_cell(lonlat = c(-179.5, -89.5), vars = 'asdfasdf')
#> Error in `[.data.frame`(NASA, , c(refcols, setdiff(names(NASA), refcols))) :
#>  undefined columns selected

Seems like failing earlier (before the GET request) if allowed vars is known ahead of time would be best

  • as a possible thing for later, at least with the region fxn, seems they can take a long time to download, maybe caching them would be helpful to users?
  • with enddate = Sys.Date() is there a possibility this will fail if it's a date that the data isn't ready yet? is there at least a good failure mode when that happens?
adamhsparks commented

@sckott, can you add some detail to your first point, "unless you have some edge case stuff, would it not be easier to construct a named list of query params to pass to GET"? I don't understand what you're suggesting here.

Also, do you have any ideas for caching the data with the region fxn? I'm trying to figure something out, I got nothin. Does httr offer any methods for caching data or capturing data from a failed download?

I've covered all of your other points listed here already.

check user-entered vars
ropensci/nasapower@034ab88

split GET and content
ropensci/nasapower@2f4f699

handle endate values
ropensci/nasapower@a302a20

I've also introduced a check and reformat for user-entered stdate and endate values
ropensci/nasapower@c8830d8

  • ๐Ÿ‘1
sckott commented

e.g. instead of this pattern https://github.com/adamhsparks/nasapower/blob/master/R/get_cell.R#L111-L135 can do instead list(lon = longlat[1], lat = lonlat[2], ...), etc. and pass to GET as first param just the base URL, and pass that list to query

No, there's nothing built in for caching. There's lots of potential solutions for caching. A pattern I use is https://cran.rstudio.com/web/packages/hoardr/ which I think you use in a few of your pkgs? But there are other solutions: using rappdirs by itself, etc. I think the important part is whether or not you think it's needed (you know better than I), then at least make sure to use the right paths on the users machine (via rappdirs), and make sure to remove any files from a failed download so that you don't end up reading in later a cached file that doesn't have what you expect to have in it. Also, CRAN doesn't like it when you write to the disk in tests/examples that run on on cran checks (hoardr makes this easier by allowing to write to a tempdir, which they are okay with)

what do you mean by "capturing data from a failed download"

adamhsparks commented

Ah, sorry about the caching. I totally understand. My mind had just gone somewhere else with trying to capture data being downloaded, my example of downloading the entire grid for one day takes a while, and thinking if the download was interrupted how to capture that so as to not have to start over again. Might not be possible.

Sure, I can integrate hoardr into this package, easily done.

I've glanced at the httr documentation. I think I understand the list, GET and query structure.

Thanks for the suggestions and clarification.

adamhsparks commented

@sckott, I've implemented the use of query. I can't seem to get it to allow for multiple values with the same name though. All of the weather parameters are the same key, "p". I can create a list with all ten parameters listed, but query only seems to return the first and drops the rest. I've worked around it with a bit cleaner bit of code that still involves paste than what I had used. Overall, it does make the code cleaner and easier to follow. Thank you for that suggestion. I now know more about interacting with web APIs.

I've been thinking about the use of hoardr or rappdirs for this package. I don't know if there is much merit to it, but could be convinced otherwise. From my thinking, it's not hard to just use write.csv or fwrite or write_csv or ... and save the data since it's already in a tidy data frame. I was really aiming for this one to be a simple interface to just download and format the data, after that it's up to the user.

Some difficulties, if the user makes multiple requests, e.g., one for Queensland and another for Australia, then you could save time not querying Qld, since it's nested in Australia. However, it would require a fair bit of work with some spatial packages to check what's present and what's requested and getting them to align since you can request pretty much any combination of lat/lon values from the API and it will give you the data for those values.

sckott commented

Right, repeated params are a bit trickier. Glad it works.

Totally up to you on the caching, makes sense to me what you describe to not do the caching.

  • ๐Ÿ‘1
sckott commented

@adamhsparks still working on changes?

adamhsparks commented

Yeah, I've been sidetracked a bit. I don't know when the new API goes live and the last time I had the chance to test it the site was down.

I do know that the current version of the package does work, I've had feedback from someone using it with another one of my R packages that models rice diseases. They're steps ahead of me, I've not even done that yet.

I'll try to devote some time to it later this week.

sckott commented

Thanks for the update!

adamhsparks commented

Hi @sckott, I know you've seen that I fixed a bug in the master branch and updated the currently released version this week.

I've also spent a few minutes on the devel version working with the new API. I've successfully run a test and retrieved data using the new API with crul! I need some time to tidy up and write tests since this is basically a complete rewrite of the package, but I'm happy to say that I'm making progress, albeit slowly.

sckott commented

Thanks for the progress update, all sounds good

adamhsparks commented

Just to update you, @sckott, I'm short on time right now to work on this. However, any time I've tried to query multiple vars it fails telling me I have an incorrect var specified. However, if I query any of the same vars singly, it succeeds for all of them.

I need some more time to really focus on this, but I likely won't have that until sometime next month. Is that OK?

sckott commented

Sure thing, no problem. It's nice to have the updates.

adamhsparks commented

Hi @sckott, I'm still working on this. I had some time this weekend to devote to it and have been writing tests. I'm nearly 1/2 done with the tests and then need to rewrite the vignettes.

Sorry for the delays. "Real" work keeps getting in the way. I've also had to make some decisions about how/what data to access. I can't find a good way to reliably access the ESRI REST API for raster data through R yet. So I will just be focusing on the regular single point/regional API that Power provides, for now with hopes that in the future I can sort the access to raster data directly.

sckott commented

sounds good, thanks @adamhsparks

adamhsparks commented

OK! After several months hiatus, I think we're back in business.

I've completely rewritten the nasapower package to utilise the new NASA-POWER API that was recently released and use the new POWER data. In essence, this is a new R package. If there is any code carryover it's extremely minimal due to the new processes and interfaces. I did manage to keep one vignette, the use-case, the same though.

Unfortunately due to the way the API handles requests, I can't fetch a whole country like I could with the previous version (in the vignette). This may be possible if I (or someone else smarter than me) can figure out how to access the ESRI REST API. For now you can query a single point, a region in a 5x5 arc degree area for daily data or global climatology data.

I think I'm ready for the editors to decide what to do with this now that it's been reviewed once and completely rewritten due to changes in the source immediately after the original reviews.

UniversalTourist commented

Hello All,

I think I'll have time to review again after next week (I'll be away from home for holiday). I'll let you know if I encounter any problems!

Cheers,
Hazel

sckott commented

Thanks @adamhsparks for the update.

I dropped the status back to 3, reviewers assigned, I think we should get new reviews. Looks like @UniversalTourist already said she can review again. @alisonboyer could you review this new version of the package? If not just let me know

adamhsparks commented

Very good, I agree, it needs new reviews since it's basically a new package.

Thanks for your patience, @sckott.

  • ๐Ÿ‘1
adamhsparks commented

I've updated the initial post here to reflect the newest version of the package now.

alisonboyer commented
  • ๐Ÿ‘1
sckott commented

@UniversalTourist @alisonboyer Will you two be able to do reviews soon-ish? ๐Ÿ™ thanks so so much

adamhsparks commented

Sorry, Scott, they can have more time.

NASA just made changes to the API suddenly without warning. The formerly proper CSV file now is no longer rectangular, I need to rethink how the data is ingested and I'm leaving for two weeks of travel on Friday. I've got some ideas in mind but it'll take me a while before I can get to it.

sckott commented

Okay, thanks @adamhsparks for the heads up.

UniversalTourist commented

My plan was to start this weekend -it was heavy two weeks- but OK, I'll wait to hear from you!

adamhsparks commented

Sorry about that. I think we're ready now again for review. The POWER team changed the response of the API to include some extra metadata and didn't alert anyone interfacing with the API. I've now made the changes to be able to gather that metadata but also fixed the issue that precluded the data from being read due to changes in the response.

  • ๐Ÿ‘2
sckott commented

thanks @adamhsparks ping @UniversalTourist and @alisonboyer ^^

  • ๐Ÿ‘1
UniversalTourist commented

I finally have time today to finish the review! I just wanted to let you know about that :)
Many things has changed apparently! :)

UniversalTourist commented 
## Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

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

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: It took almost, totally, 2 hours to check everything.

Review Comments

  • Installation:

With MacOS:
I used macOS High Sierra, Verison 10.13.3 and R version 3.5.0 (Joy in Playing). I was able to install the package from Github without any problem by devtools.

  • Code Overall:
    These package has one function to get data for a cell or for a region. It has nice structered, easy to follow and also nice explained with comments. Also two new functions which help using nasa power data for modelling purpose. I don't have any experience on ICASA file for use in DSSAT modelling but I used APSIM files for an archaelogical modelling project two years ago. And I found it very useful.

  • rOpenSci criteria:
    Package name is really nice. Function and variable naming is proper. Licence and Code of Conduct are available.

  • Readme:
    Readme.rmd is clear and easy to follow steps for installations.

  • Errors and console messages:
    They are all clear.

  • R CMD check:

I only get these error and warning. I always see that error when I'm running R but I don't know the reason for warning.

Status: 1 ERROR, 1 WARNING
See
  '/private/var/folders/9g/cqymx4b10nd1_45vjh6zq_040000gn/T/RtmpqYWghO/nasapower.Rcheck/00check.log'
for details.

R CMD check results
0 errors | 1 warning  | 0 notes
checking DESCRIPTION meta-information ... WARNING
During startup - Warning messages:
1: Setting LC_CTYPE failed, using "C" 
2: Setting LC_TIME failed, using "C" 
3: Setting LC_MESSAGES failed, using "C" 
4: Setting LC_MONETARY failed, using "C" 

ERROR
During startup - Warning messages:
1: Setting LC_CTYPE failed, using "C" 
2: Setting LC_TIME failed, using "C" 
3: Setting LC_MESSAGES failed, using "C" 
4: Setting LC_MONETARY failed, using "C"
  • ๐Ÿ‘2
adamhsparks commented

Hi @UniversalTourist, try suggestions from this SO thread to see if it helps?
https://stackoverflow.com/questions/9689104/installing-r-on-mac-warning-messages-setting-lc-ctype-failed-using-c

As you've indicated, it always happens, so it shouldn't be related to nasapower.

  • ๐Ÿ‘1
sckott commented

@alisonboyer can you get another review in within a week ? if not no worries and we'll move on

alisonboyer commented
sckott commented

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

Good idea to add guidelines for users on how to contribute. See e.g. https://github.com/ropensci/dotgithubfiles/

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: 1

Review Comments

awesomeness:

  • nice work!
  • nice failure behavior!

other comments:

  • duplicate data in REAMDE surface solar energy climatology data data
  • you could be a bit more detailed on your parameters man file, e.g., A list with 141 objects. What kind of objects? And it looks like the list is named by something useful?
  • Having examples for all functions is good, but the examples are pretty sparse, only 1 for each fxn.
  • including @return for each fxn would help users know what to expect
  • nice use of parameter checking in your fxns. for logical parameters, there doesn't seem to be checks. so e.g., a user could do get_power(meta = 5), though unlikely could happen i guess
  • it's probably a good idea for examples and tests to write to tmp dir instead of a non-temporary user path, e.g., dsn = "~/Documents"
  • this failure behavior isn't great when dsn and or file_out not given (same for create_met):
create_icasa(lonlat = c(151.81, -27.48),
             dates = c("1985-01-01", "1985-12-31")
)
Error in if (substr(file_out, nchar(file_out) - 3, nchar(file_out)) !=  : 
  argument is of length zero
  • ๐ŸŽ‰1
adamhsparks commented

Thanks @sckott, I'll address your comments shortly.

  • ๐Ÿ‘1
adamhsparks commented

@sckott, did you have any thoughts on the fact that get_power() returns a list or a data frame depending the meta flag? I can't think of a better way to handle it aside from inserting the meta-information into the data frame along with the data, is that a good idea or necessary or is how I've handled it the best way to, what do you think?

sckott commented

I'm sure I have some fxns that return different objects depending on user inputs, but it definitely seems better to avoid that. Maybe one of these two optoins:

  • put the metadata in attributes in the data.frame, then user can access them with attributes or some custom thing
  • make a custom print fxn where the metadata is printed on top of the data.frame sort of like tibbles do, then the object itself is still a data.frame
  • ๐Ÿ‘1
adamhsparks commented

I've some ideas of what can be done with it. I'll think on it some more and see what I can implement.

At this point I think I've addressed everything else you've pointed out above.

  • ๐Ÿ‘1
sckott commented

I've some ideas of what can be done with it. I'll think on it some more and see what I can implement.

should I wait on this for this submission, or do you mean more in the long term?

adamhsparks commented

No, please wait, I've pushed a custom print() method last night that includes an attribute header to the devel branch. Polishing it, will be done within 48 hrs I'd guess?

sckott commented

will do.

  • ๐Ÿ‘1
adamhsparks commented

Hi @sckott, I think it's ready for you to check and hopefully accept.

I've found and squashed several more bugs while dealing with the metadata and your suggestions.

cheers!

  • ๐Ÿ‘1
sckott commented

@adamhsparks a few things:

  • the example for create_icasa has file = ICASA_example.txt where the file name is not quoted, R is looking for an object named ICASA_example.txt
  • for the create_met example you have dsn = tmpdir(), which sholuld be tempdir() -
  • the get_power 2nd example doesn't work either. you have temporal_average as a required parameter, but don't supply anything to it in the 2nd example.
  • Also about get_power. it's not a good idea to have a required parameter (w/o a default) after a non-required parameter. that is, you have function(community, lonlat, pars, dates = NULL, temporal_average), whereas function(community, lonlat, pars, temporal_average, dates = NULL) would make more sense b/c all required paramters are first, then the last is optional

Its essential that your examples run successfully. a good way to do this is have them run on travis, e.g. https://github.com/ropensci/wellknown/blob/master/.travis.yml#L28 Please make sure your examples work moving forward

adamhsparks commented

Thanks @sckott, I'll address these issues.

I've not seen that Travis configuration before, that's useful!

  • ๐Ÿ‘1
adamhsparks commented

Hi @sckott, after that embarrassing blunder, I've corrected the examples and a few other issues to align with rOpenSci guidelines if you'd like to have a look.

Thanks again for the suggestion with Travis to build examples, it caught yet another error for me even after I'd fixed everything once!

  • ๐Ÿ‘1
sckott commented

nice, glad it was helpful.

having another quick look

sckott commented

Approved! Thanks again for your submission @adamhsparks !

  • Please transfer the package to ropensci- You'll be made admin once you do.
  • 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
  • ๐ŸŽ‰1
stefaniebutland commented

Hi @adamhsparks ! You know the blog post drill from https://ropensci.org/blog/2017/04/04/gsodr/ ๐Ÿ˜„. Are you interested in doing one, or a tech note on nasapower?

Tech Notes can be published at any time, while we publish blog posts about packages weekly. Right now Tuesday 2018-10-16 or subsequent Tuesdays are available for blog posts. If you're interested, let me know your preferred publication date and then please submit via pull request a week prior. Guidelines are here: https://github.com/ropensci/roweb2#contributing-a-blog-post

  • ๐Ÿ‘1
adamhsparks commented

Excellent! I'm tidying up a few remaining issues and will transfer it over shortly.

@sckott and @stefaniebutland, yes, we should do a proper blog post about this one. I'll get something together and submit a PR next month.

Thank you all, once again, for the support and motivation. This package has been a long-time coming, but I'm super happy with the condition it's now in before it even reaches CRAN (which will hopefully happy soon as well).

adamhsparks commented

Ok, I've transferred ownership and once again lost the ability to edit the webpage and the tags associated with the repository so the webpage link incorrectly points to my personal repository, still, as I forgot to change that before transferring it over.

Can I get the right permissions to edit the repository details, etc.?

ta!

sckott commented

nice work adam!

  • ๐ŸŽ‰1
sckott commented

done