Skip to content
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

New function (as.double2), plus documentation and tests. #1

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

knapply
Copy link
Collaborator

@knapply knapply commented Feb 3, 2021

This should be a drop-in replacement for as.double() for character vectors. Take a peek when you get a chance and see if there's some case or behavior that's still missing.

N <- 1e6
set.seed(8675309)
strings <- c(
  "NaN", "-NaN", "nan", "-nan", "Inf", "-Inf", "inf", "-inf", "infinity", "-infinity", NA_character_,
  paste0(letters, LETTERS, month.abb, month.name),
  as.character(seq.Date(as.Date("1970-01-01"), as.Date("1970-02-01"), by = 1L))
)
input <- sample(c(
  paste0(" \r\n\t\f\v", c(0.0, sqrt(seq(1, N))), " \r\n\t\f\v"),
  rep(strings, 3)
))

library(RcppFastFloat)
suppressWarnings(
microbenchmark::microbenchmark(
  as.double(input),
  as.double2(input),
  check = "equal"
)
)
##> Unit: milliseconds
##>               expr      min       lq     mean   median       uq      max neval
##>   as.double(input) 667.3757 684.0287 720.9842 696.3353 743.7095 968.3394   100
##>  as.double2(input) 130.9257 137.0082 158.7741 141.0736 150.5398 584.5855   100

@eddelbuettel
Copy link
Owner

I may let this sit and stew til we're through newbies at CRAN but will then get to it presto.

@@ -17,5 +17,6 @@ LinkingTo: Rcpp
Suggests: tinytest
URL: https://github.com/eddelbuettel/rcppfastfloat/
BugReports: https://github.com/eddelbuettel/rcppfastfloat/issues
RoxygenNote: 6.0.1
RoxygenNote: 7.1.1
Roxygen: list(markdown = TRUE)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change it now, but I have a slight preference for the old markdown 6.0.1 which I have in an opt-in directory. It does not do the stooopid dance of forcing a recompilation and rebuild only to deal with Rd creation. And with that I often say no to markdown too. (At least in stull-small projects such as this. Exceptions can be made and are made.)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also realize that it is awkward with collaborators who won't have 6.0.1 around. Ah well. RStudio lacks flexibility in invoking alternate converters, sadly.

Copy link
Collaborator Author

@knapply knapply Feb 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, just help remind me if I forget/don't get to it before you're looking to ship to CRAN.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably keep it. I am on the fence on md-in-Rd too. It's on in some projects, I am just not too consisten. No worries right now. This all looks svelte as usual.

Copy link
Owner

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're on CRAN now so I'll merge this. Looks good, as usual :)

@eddelbuettel eddelbuettel merged commit 9b15cf2 into master Feb 5, 2021
eddelbuettel added a commit that referenced this pull request Feb 5, 2021
@eddelbuettel eddelbuettel deleted the feature/asdouble2 branch February 13, 2021 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants