-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
cmipr - Client for Coupled Model Intercomparison Project (CMIP) data #99
Comments
Editor checks:
Editor commentsThank you for your submission @sckott!
It is good practice to
? write unit tests for all functions, and all package code in general.
80% of code lines are covered by test cases.
R/cmip_fetch.R:24:NA
R/cmip_fetch.R:25:NA
R/cmip_fetch.R:26:NA
R/cmip_read.R:41:NA
R/cmip_read.R:43:NA
... and 8 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
tests\testthat\test-cmip_fetch.R:4:1
tests\testthat\test-cmip_read.R:4:1 where the long lines are only due to paths so you could only add "# nolint" at the end of these lines. 😉
|
thanks @masalmon will fix those things |
@masalmon okay, those 2 thigns fixed |
Thanks for accepting to review this package @cvitolo and @bpbond ! As a reminder here are links to the recently updated reviewing and packaging guides and to the review template. |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
I think that the readme should mention the netcdf library as external dependency.
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 4 Review Comments@masalmon @sckott in my opinion this package is well written and it does what it says. I only have very few comments which I filed as issues, also reporting specific examples. The main comment is that all the functions contain basic documentation but it would be great to have some more information. Once there is a more in-depth documentation, you could use Also, please consider mentioning the NetCDF library as external dependency as ncdf4 is required by the raster package to handle nc files in your examples. |
Thanks a lot for your review @cvitolo ! |
thanks for your review @cvitolo ! 😸 😃 |
@masalmon @sckott my apologies! |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Functionality
Final approval (post-review)
Estimated hours spent reviewing: 2 Review CommentsThis package provides an interface to download (a subset of) Coupled Model Intercomparison Project (CMIP, see Taylor et al. 2013) files from Overall, this package is well designed and well structured. The R code is clean and clearly written and formatted, although a bit light on comments. Test coverage is good (although see ropensci-archive/cmipr#15). In generally the package complies with the ROpenSci and Mozilla guides. Documentation is thin--especially the function descriptions, which are extremely short and should be fleshed out--but what is there is clear and well written. R CMD CHECK passes with no errors, warnings, or notes. A few suggestions:
The biggest problem is that the package needs to make clear it's using My test environment:
|
Thanks a lot for your review @bpbond 😃 |
thanks for your review @bpbond ! So I can get a complete view of CMIP data:
Do you think the folks behind |
Other places to get data - well, the canonical 'place' is the Earth System Grid Federation but that's been a source of huge frustration for lots of people (which is probably why LLNL established its ftp server). I'm sure there are other private repositories too (we have one, but it's sealed behind a firewall that I can't control). The |
thanks! that info is really helpful. I'll follow up on the rsync source |
Just occurred to me - one other alternative would be to ask LLNL if they'd be willing to expand their CMIP data holdings. |
Ah, good point, I'll ask them that |
Was told that public access not possible - but pointed me towards https://github.com/Prodiguer/synda - that not useable here, but sounds like there might be some open web services that synda uses that we can use, e.g., at this location http://esgf-index1.ceda.ac.uk/esg-search/search |
@bpbond heard back from ATMOS folks have you ever used the Synda command line tool? https://github.com/Prodiguer/synda They suggested to have users use that on the command line and then just read data in within R - as opposed to doing the downloads ourselves here. thoughts? He said:
may be true, but you said it's only partial data, yes? |
Hey Scott. No, I don't know Synda–huh, interesting, thanks. I agree that the original CMIP5 data can be a mess...but yeah, LLNL seems (as far as I could see) to be extremely partial: as in, out of the hundreds of CMIP5 output variables, they only seem to have two or three.
|
@bpbond I think we really just need to re-create what synda does in R cause a do X in CLI then Y in R isn't ideal - i'll play around - will probably need to put this review on hold for a while 😸 |
@sckott any update on this package? 😉 |
now that |
Great, I hope it will be a fruitful and interesting process! 🍀 |
@sckott any progress? In any case you can add the peer-review badge to
|
@maelle will do the badge blarg, no sorry, no progress. will try to get to very soon. |
@sckott any news? 😉 |
sorry @maelle about the delay! I have on to do list to look into the new data source options, and have not done so yet. will try to get it done asap |
Thanks for the update! 😸 |
👋 @sckott re-visiting the issue, any news? 😺 |
ooofffff - not yet. is on my to do list but keeps getting pushed off the end. |
@bpbond Finally trying to get back to this. Looking at api and the python client http://esgf-pyclient.readthedocs.io/en/latest/ and trying the same in R (been using base url https://esgf-index1.ceda.ac.uk so far), but it seems like I can only use the search API freely, but many data files I've tried so far are behind login walls. Do you know what it takes to get access to ESGF data? Is it a valuable contribution to provide an R interface to the search API at least? |
Hey @sckott . That's true as far as I know--downloading data requires an ESGF login account; note that ESGF is a source of huge frustration to many researchers. I think even exposing the search API through R would be valuable, yes! Anything to make the system easier to use... |
Thanks @bpbond - Okay, sounds like best approach moving forward is to just give access to any freely available cmip data I can, and have a search API interface. Then if folks with access to the actual data files want to contribute later on that's great |
@maelle I think i'm going to not work on this package anymore - too many pkgs and this is a tough one to sort out, so we can close this and i'll mark the repo as abandoned. |
Oops I had missed this! |
Summary
Client to work with CMIP data - downscaled climate and hydrology projections. Package lists avail. files, downloads and caches, and reads into raster objects.
https://github.com/ropenscilabs/cmipr
People using climate projections for research - or studying climate change itself.
https://github.com/JGCRI/RCMIP5 deals with CMIP data, but AFAICT only handles data after you have it on your machine. So that package and this one could be used together.
Requirements
Confirm each of the following by checking the box. This package:
Publication options
paper.md
with a high-level description in the package root or ininst/
.Detail
Does
R CMD check
(ordevtools::check()
) succeed? Paste and describe any errors or warnings: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:
maybe https://github.com/bpbond since author of the above mentioned pkg
The text was updated successfully, but these errors were encountered: