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

Collect output in an accumulating stack #166

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Collect output in an accumulating stack #166

merged 1 commit into from
Jun 21, 2024

Conversation

hadley
Copy link
Member

@hadley hadley commented Jun 21, 2024

I think this slightly simplifies the existing code, and as you'll seen in upcoming PRs, allows us to tease apart concerns to yield code that's a little easier to understand.


You might be surprised to see me repeatedly growing a list, but this is quite performant in modern versions of R:

f <- function(n) {
  output <- watchout()
  for (i in 1:n) output$push(1:10)
  output$get()
}
bench::mark(
  f(10),
  f(100),
  f(1000),
  f(10000),
  check = FALSE
)[1:3]
#> # A tibble: 4 × 3
#> expression      min   median
#> <bch:expr> <bch:tm> <bch:tm>
#> 1 f(10)      286.02µs 368.92µs
#> 2 f(100)     352.27µs 442.66µs
#> 3 f(1000)      1.03ms    1.2ms
#> 4 f(10000)     7.79ms   8.49ms

(And for older versions of R, I don't think it would the typical length of output is going to cause a performance bottleneck)

I think this slightly simplifies the existing code, and as you'll seen in upcoming PRs, allows us to tease apart concerns to yield code that's a little easier to understand.
@hadley hadley requested a review from cderv June 21, 2024 09:05
invisible()
}

# record whether or not we've seen an error
Copy link
Member Author

Choose a reason for hiding this comment

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

This error handling is a little inelegant, but I'm pretty sure it'll go away in a future PR.

Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

This makes sense. I did not know the performance had improved. It is cleaner to use this push() logic at least for reading code and understanding what is happening!

@hadley hadley merged commit 6f07825 into main Jun 21, 2024
13 checks passed
@hadley hadley deleted the output-stack branch June 21, 2024 15:33
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.

2 participants