-
-
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
rrricanes #118
Comments
Thanks for your submission @timtrice ! 😸 I have two questions
Tests results > devtools::test()
Loading rrricanes
Loading required package: testthat
Attaching package: ‘testthat’
The following object is masked from ‘package:dplyr’:
matches
Testing rrricanes
Test base functions.: .........................
Storm Discussions (discus): ...........................12
|====================================================================================|100% ~0 s remaining ....
Forecast/Advisory Products (fstadv): 3
Test getting storm data.: .
|====================================================================================|100% ~0 s remaining ..
Position Estimates (posest): .................
|====================================================================================|100% ~0 s remaining 4
|====================================================================================|100% ~0 s remaining .....5
Scrapers: .................
Update (update): .................
|====================================================================================|100% ~0 s remaining .
Failed -------------------------------------------------------------------------------------------------------
1. Error: Test discus() (@test_discus.R#54) ------------------------------------------------------------------
character string is not in a standard unambiguous format
1: discus(link = url) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/tests/testthat/test_discus.R:54
2: scrape_header(contents, ret = "date") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/discus.R:71
3: scrape_date(header) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/scrapers.R:234
4: as.POSIXct(x, tz = "Etc/GMT+4") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/scrapers.R:162
5: as.POSIXct.default(x, tz = "Etc/GMT+4")
6: as.POSIXct(as.POSIXlt(x, tz, ...), tz, ...)
7: as.POSIXlt(x, tz, ...)
8: as.POSIXlt.character(x, tz, ...)
9: stop("character string is not in a standard unambiguous format")
2. Error: Test get_discus() (@test_discus.R#71) --------------------------------------------------------------
character string is not in a standard unambiguous format
1: get_discus(link = url) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/tests/testthat/test_discus.R:71
2: purrr::map(filter_discus(products), discus) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/discus.R:41
3: .f(.x[[i]], ...)
4: scrape_header(contents, ret = "date") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/discus.R:71
5: scrape_date(header) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/scrapers.R:234
6: as.POSIXct(x, tz = "Etc/GMT+4") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/scrapers.R:162
7: as.POSIXct.default(x, tz = "Etc/GMT+4")
8: as.POSIXct(as.POSIXlt(x, tz, ...), tz, ...)
9: as.POSIXlt(x, tz, ...)
10: as.POSIXlt.character(x, tz, ...)
11: stop("character string is not in a standard unambiguous format")
3. Error: (unknown) (@test_fstadv.R#8) -----------------------------------------------------------------------
character string is not in a standard unambiguous format
1: get_fstadv(link = url.al011998) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/tests/testthat/test_fstadv.R:8
2: purrr::map(filter_fstadv(products), fstadv) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/fstadv.R:157
3: .f(.x[[i]], ...)
4: scrape_header(contents, ret = "date") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/fstadv.R:210
5: scrape_date(header) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/scrapers.R:234
6: as.POSIXct(x, tz = "UTC") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/scrapers.R:152
7: as.POSIXct.default(x, tz = "UTC")
8: as.POSIXct(as.POSIXlt(x, tz, ...), tz, ...)
9: as.POSIXlt(x, tz, ...)
10: as.POSIXlt.character(x, tz, ...)
11: stop("character string is not in a standard unambiguous format")
4. Error: (unknown) (@test_prblty.R#9) -----------------------------------------------------------------------
character string is not in a standard unambiguous format
1: get_prblty(al1998[1]) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/tests/testthat/test_prblty.R:9
2: purrr::map(filter_prblty(products), prblty) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/prblty.R:25
3: .f(.x[[i]], ...)
4: scrape_header(contents, ret = "date") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/prblty.R:52
5: scrape_date(header) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/scrapers.R:234
6: as.POSIXct(x, tz = "Etc/GMT+4") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/scrapers.R:156
7: as.POSIXct.default(x, tz = "Etc/GMT+4")
8: as.POSIXct(as.POSIXlt(x, tz, ...), tz, ...)
9: as.POSIXlt(x, tz, ...)
10: as.POSIXlt.character(x, tz, ...)
11: stop("character string is not in a standard unambiguous format")
5. Error: 1998, Tropical Storm Alex, Advisory 26 (@test_public.R#38) -----------------------------------------
character string is not in a standard unambiguous format
1: expect_identical(public(al.1998.alex.products.public[25]) %>% dplyr::select(Status) %>% dplyr::first(), "Tropical Disturbance") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/tests/testthat/test_public.R:38
2: identical(object, expected)
3: public(al.1998.alex.products.public[25]) %>% dplyr::select(Status) %>% dplyr::first()
4: eval(lhs, parent, parent)
5: eval(expr, envir, enclos)
6: public(al.1998.alex.products.public[25])
7: scrape_header(contents, ret = "date") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/public.R:70
8: scrape_date(header) at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/scrapers.R:234
9: as.POSIXct(x, tz = "Etc/GMT+4") at C:\Users\msalmon.ISGLOBAL\Documents\rrricanes/R/scrapers.R:156
10: as.POSIXct.default(x, tz = "Etc/GMT+4")
11: as.POSIXct(as.POSIXlt(x, tz, ...), tz, ...)
12: as.POSIXlt(x, tz, ...)
13: as.POSIXlt.character(x, tz, ...)
14: stop("character string is not in a standard unambiguous format")
DONE =========================================================================================================
Don't worry, you'll get it.
s <- sapply(x, function(z, n = names, l = link, m = msg){
f <- paste("get", z, sep = "_")
res <- do.call(f, args = list("link" = l, "msg" = m))
if (!is.null(n[[z]])) {
assign(n[z][[1]], res, envir = .GlobalEnv)
} else {
assign(z, res, envir = .GlobalEnv)
}
}) |
@maelle , thank you for taking the time to review my project! Regarding the code in question, it has been replaced with the following: ds <- purrr::map(products, .f = function(x) {
sprintf("get_%s", x) %>%
purrr::invoke_map(.x = list(link = link)) %>%
purrr::flatten_df()})
names(ds) <- products
return(ds) This is in branch Regarding the tests; I apologize for them not being enabled. At one point earlier this week I had to disable them due to the NHC archives being down for some annual archive pages. This seems to have been resolved now. Tests have been added to both Travis and Appveyor. A side note to that last point; I have noticed there are timeouts when accessing the NHC archives. This has been verified on numerous occasions with Travis and Appveyor during tests. I have a feature request pending to add handling for this. Additionally, in 0.2.0, |
@timtrice thanks, nice work! For review (and my editor checks :-) ) will master be updated? Great on having already processed datasets, but I see that repo has no licensing information. I'm not experienced at all in licensing issues but I guess it'd be at least nice to add where this data comes from etc. |
Now that I could check the package (on the right R version) here is something more formalized. Editor checks:
Editor comments
── GP rrricanes ───────────────────────────────────────────────────────────────────────────────────────────────
It is good practice to
✖ write unit tests for all functions, and all package code in general. 93% of code lines are
covered by test cases.
R/base.R:74:NA
R/base.R:75:NA
R/base.R:76:NA
R/base.R:77:NA
R/base.R:78:NA
... and 64 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
R\base.R:84:1
R\scrapers.R:83:1
tests\testthat\test_scrapers.R:46:1
currentlby get_fstadv.Rd:18
dataframej build_archive_df.Rd:20 Reviewers: @jsta @robinsones |
@maelle, thank you for the compliment! Very much appreciated. The In hindsight I should have incorporated bug fixes into master. It was one of those things that I had already made new changes and started just hammering out bugs - before I knew it I lost track of where I was. This is the first project I'm trying to incorporate R package rules and github best practices along with what I know needs to be done. Some of it (as your example above) is just a sloppy "get it working" attempt before I get back to making the code cleaner. Regarding |
@timtrice sorry to have posted so many comments in a row. Should we put the review on hold until you've merged everything with master? This way you'd get a review on the final product. :-) |
@maelle , No problem regarding the comments. Let me push the fixes to 0.1.2; I'll hold back the GIS products until I can determine the best direction. I'll post an update later today or tomorrow morning when it's complete. The tests do take forever. This is because of the scraping; particularly the forecast/advisory products. There are minor variations in some of the advisories so I have to make sure that one regex pattern change here doesn't affect other advisories. That is another issue I'm also trying to determine better methods. Edit: to be clear those notes should no longer exist in this new release. Thank you |
Ok so I'll re-do the editor checks once you've updated this thread! :-) |
@timtrice I added the results of |
I have released 0.1.2 into the master and develop branch. Local checks and tests pass. I'm having issues passing them on Appveyor. Travis has passed. Appveyor wants to fail 2 or 3 tests in because of timeout issues. I hoped to have resolved this by adding functionality to reattempt data collection on timeout but it apparently isn't enough. At least, for testing. I'm not sure what else I can do to resolve those type of issues. I'll keep re-building the commit if it continues to fail but hopefully it'll catch a good run and finally go through. (Edit to note all tests in Appveyor finally passed) Thank you again for taking the time to review my project and I look forward to hearing back from the community about it. Tim |
@timtrice awesome, and could you identify why they previously failed? |
@timtrice Best loading message ever! 😁 I updated the editor checks. I'm now looking for reviewers. |
@maelle , In these instances (and very common), when the tests are checked or vignettes are built the functions are scraping the National Hurricane Center archives. Very often a timeout is reached though I have gotten other odd errors I haven't researched yet. In Release 0.1.2, I wrapped the initial extraction function in a check: get_url_contents <- function(link) {
# Try to establish connection three times with timeout of 3 seconds
max_attempts <- getOption("rrricanes.http_attempts")
if (max_attempts > 5) max_attempts <- 5
for (i in seq(1, max_attempts)) {
safe_GET <- purrr::safely(httr::GET)
contents <- safe_GET(url = link,
httr::timeout(getOption("rrricanes.http_timeout")))
if (!is.null(contents$result))
return(xml2::read_html(x = contents$result))
}
stop(contents$error$message, call. = TRUE)
} So, there are two options user can set; The constant issues with the archives is why I built I'm open to any alternatives or ideas you and others may have. It is very annoying and frustrating, especially with Appveyor that may get through the first four tests successfully but fail on the last; in which I have to rebuild the entire thing. I am also looking at implementing data.world in some way to make it easier. As the plans are to add GIS data, reconnaissance data and others, I know I need to develop a good solid data plan going into future releases. Thank you, Tim Trice |
Thanks @maelle, I'll take a look at it. I'm trying a simple Sys.sleep right now. Here is the latest example of a fail in Travis: https://travis-ci.org/timtrice/rrricanes/jobs/241505560
Just constant... The NHC webmaster is aware, yes. Addtionally, the data is considered public domain. The National Weather Service states:
I've tried to document thoroughly where the data originates and, to the best of my knowledge, have made no implicit or explicit claims the data is mine or that it is restricted. |
Reg. the licensing stuff I'd suggest we wait for the reviewers (that I'm currently looking for) whether they have suggestions, and if needed we can ask the community at large what's best practice. :-) |
Apologies if I keep moving the goal posts, so-to-speak, but I found errors in some of the tidy functions and had to rush a patch. Release 0.1.3 has been pushed to master. |
One reviewer found, seeking another one. @timtrice |
Can someone advise how I should handle new releases? I expect to be able to push a beta release of 0.2.0 today into master but I want to make sure this will not cause any issues with the reviewing stage. |
@jsta, thank you. I corrected the package title and updated the documentation. |
Looking at both packages one last time before approving, sorry for the delay.
Despite these few remarks the 2 packages are approved! 🎉 Thanks a lot to the reviewers @jsta and @robinsones and to you @timtrice ! To-dos
|
Very glad to hear the news~ I have removed you as a reviewer to I have also made some modifications to I will wait to push that to master until you provide the README links. Once I push to master I'll transfer both repos. Regarding the license, I never really got a consensus on this. The discussion, if I recall correctly, centered more around if it mattered as it was not a CRAN package. But, there wasn't a definitive, "This is/is not the correct license for this type of package". Regarding |
@timtrice nice work!
|
One other question for clarity; am I transferring the drat repo as well? Or just |
Link to badge added! |
@maelle , For the most part I'm done. I need to update the pkgdown docs for But, the URL in the description for each repo, I can't change. I guess I'm not an admin for my repo anymore? It still points to the old repo address for the pkgdown documentation which I need to update. |
Just made you admin of both repos, sorry about that |
Carl and I discussed about drat - we have our own as you may know, and so once these two pkgs are in ropensci org, they will be included in our drat - so i'd say not move the drat repo |
Thanks @sckott (and Carl)! |
@timtrice I forgot to ask whether you'd like to write a a blog post about your package for rOpenSci blog, either a short-form intro to it (https://ropensci.org/tech-notes/) or long-form post with more narrative about its development. ((https://ropensci.org/blog/). Let me know if you are interested. |
@maelle , that'd be fantastic, yes! |
@timtrice if you submit the package to CRAN before R3.5, please build it on R-devel in order not to get any note because of the "rev" role. :-) |
@maelle , I've added introducing_rrricanes gist for review per your previous comment. If I'm on the wrong track with the purpose of this, please let me know. Or if you have additional suggestions, of course, I'm all for it. Thank you for your help! |
You mean for the blog post? Also poking @stefaniebutland then. And reading it now, many thanks! |
Nice to see this text being written! Some comments:
|
@timtrice Great to hear you're interested in contributing a blog post about As Maëlle mentioned, you could do either a short post for the Developer Blog that has a technical focus, or do a post for the main blog whose audience is broader. Main blog post would include more narrative about your motivation for creating the package, unmet need, how-to use, good to end with a thank you to package reviewers with links to their GitHub or Twitter, point readers to issues and what you think is next to improve the package and invite people to open or address an issue etc. Deadlines:
Practical instructions:
Which type of post are you thinking of? |
Some comments on introducing_rrricanes to add to Maëlle's:
I hope those comments help. Thank you again for doing this. Please tag me here or ping me on slack with any questions or to look at next version. |
I've added additional visuals to the next version and moved the document to it's own repo: https://github.com/timtrice/introducing_rrricanes The markdown file seems to be complete w/ images and what-not. This may be a bit longer than what was intended but I really wanted to cover the big ideas behind the package and try to explain them under the assumption that anyone using it would be unfamiliar with what the different data meant. I hope that I've accomplished this (even if a bit too thoroughly). Look forward to any input you both may have to offer. |
Hi @timtrice
|
Hello @stefaniebutland , @maelle , I wanted to give you guys an update on this. I finished the post last week and was ready to submit to roweb. However, currently the NHC website is down for GIS products making it impossible to render the Rmd file. They are aware of the issue and, I've been told are trying to correct it. But I have no idea when that will be. Basically, I'm on hold for now. |
Thanks @timtrice for the update! While your post is scheduled to go up Sep 19, I can easily move that date. Just ping me when you have submitted to roweb. |
I was finally able to render the Rmd file and submit the pull request as referenced above. |
Appveyor build fails treating warnings as errors. The warning is that License is non-standard. The license issue was debated when the package was created for @ropensci: ropensci/software-review#118 (comment) Message: > checking DESCRIPTION meta-information ... WARNING > Non-standard license specification: > CC0-1.0 + file LICENSE https://ci.appveyor.com/project/timtrice/rrricanesdata-on6xt/builds/21296022/job/n3iygk4wgn1stu00#L276 Source: https://github.com/ropensci/rrricanesdata/blob/master/appveyor.yml#L16 Fix: The issue lies in the WARNINGS_ARE_ERRORS. Per the issue below (and the latest README for kmrlmlr/r-appveyor), set this to FALSE by leaving the field empty. krlmlr/r-appveyor#79 https://github.com/krlmlr/r-appveyor/blob/ac6254e6d2e8629e037a0ddcfde985661487cd0a/README.md
Summary
What does this package do? (explain in 50 words or less):
Scrapes and parses tropical cyclone advisories and GIS data from the National Hurricane Center. Can access real-time or archived data for storms since 1998.
Paste the full DESCRIPTION file inside a code block below:
URL for the package (the development repository, not a stylized html page):
https://github.com/timtrice/rrricanes
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 extraction, data visualization, data munging, data packages
Who is the target audience?
Those with an interest in studying tropical meteorology, tracking storm developments and comparing the forecast abilities of the National Hurricane Center storm-to-storm, year-to-year.
Are there other R packages that accomplish the same thing? If so, what is different about yours?
I have searched (I believe) extensively and have not found one at all. To my knowledge the only other hurricane package in CRAN or GitHub is HURDAT - and I wrote that one, too :)
Requirements
R 3.3.3
Most tidyverse packages.
rnaturalearthdata and rnaturalerathhires will be required for release 0.1.1.
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:In release 0.1.0.1, only 2 notes about global variables. This has largely been resolved in unreleased 0.1.1.
Does the package conform to rOpenSci packaging guidelines? Please describe any exceptions:
I used a CONTRIBUTORS.md file instead of CODE_OF_CONDUCT.md.
I use
@keywords internal
for internal functionsIf 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:
The text was updated successfully, but these errors were encountered: