docs: improve documentation of prudence argument
#589
+59
−61
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maelle
commented
Feb 6, 2025
| @@ -30,22 +30,19 @@ knitr::opts_chunk$set( | |||
| Sys.setenv(DUCKPLYR_FALLBACK_COLLECT = 0) | |||
| ``` | |||
|
|
|||
| Unlike traditional data frames, duckplyr defers computation until absolutely necessary, allowing DuckDB to optimize execution. | |||
There was a problem hiding this comment.
this is explained better in one of the sections.
There was a problem hiding this comment.
Yes, but is this sentence harmful here?
There was a problem hiding this comment.
It could be discouraging to have a first sentence that's "complicated".
maelle
commented
Feb 6, 2025
| Thrifty duckplyr frames are a compromise between lavish and frugal, discussed further below. | ||
| Thrifty duckplyr frames are a compromise between lavish and frugal, discussed below. | ||
|
|
||
| ### Thrift |
There was a problem hiding this comment.
I found the previous location of this section a bit jarring.
There was a problem hiding this comment.
I want to give the readers time to digest "lavish" and "frugal".
There was a problem hiding this comment.
What happened to me when reading was that I saw it would be discussed later and thought, ok then. But when the "Thrift" section appeared, it felt misplaced.
maelle
commented
Feb 6, 2025
| @@ -198,7 +233,7 @@ flights_frugal[[1]] | |||
| ``` | |||
|
|
|||
|
|
|||
| ### Enforcing DuckDB operation | |||
| ### Side effect: Enforcing DuckDB operation | |||
There was a problem hiding this comment.
It is an important section but also does not go with the flow of the rest of the vignette.
There was a problem hiding this comment.
Good point, moving to "fallback".
Later.
There was a problem hiding this comment.
In the PR where I edit the fallback vignette, I added a section about this but that refers to this vignette.
maelle
commented
Feb 6, 2025
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 00b4546 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
|
This would be easier to review in two PRs. Will split and review individually. |
|
Other commit now in #593. |
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 3385047 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
krlmlr
force-pushed
the
prudence-rmd
branch
2 times, most recently
from
f404f17 to
3f29cba
Compare
krlmlr
reviewed
Feb 7, 2025
| @@ -30,22 +30,19 @@ knitr::opts_chunk$set( | |||
| Sys.setenv(DUCKPLYR_FALLBACK_COLLECT = 0) | |||
| ``` | |||
|
|
|||
| Unlike traditional data frames, duckplyr defers computation until absolutely necessary, allowing DuckDB to optimize execution. | |||
There was a problem hiding this comment.
Yes, but is this sentence harmful here?
|
|
||
| - **lazy materialization**: Unlike traditional data frames, duckplyr defers computation until absolutely necessary, i.e. lazily, allowing DuckDB to optimize execution. | ||
| - **prudence**: Automatic materialization is controllable, as automatic materialization of large data might otherwise inadvertently lead to memory problems. | ||
|
|
||
| The term "prudence" is introduced here to set a clear distinction from the concept of "laziness", and because "control of automatic materialization" is a mouthful. | ||
|
|
||
| ## Eager and lazy computation | ||
| ## DuckDB optimization: lazy evaluation |
There was a problem hiding this comment.
"lazy evaluation" is easy to confuse with how R handles arguments.
There was a problem hiding this comment.
oops, this should be "lazy computation", sorry.
| Thrifty duckplyr frames are a compromise between lavish and frugal, discussed further below. | ||
| Thrifty duckplyr frames are a compromise between lavish and frugal, discussed below. | ||
|
|
||
| ### Thrift |
There was a problem hiding this comment.
I want to give the readers time to digest "lavish" and "frugal".
| @@ -198,7 +233,7 @@ flights_frugal[[1]] | |||
| ``` | |||
|
|
|||
|
|
|||
| ### Enforcing DuckDB operation | |||
| ### Side effect: Enforcing DuckDB operation | |||
There was a problem hiding this comment.
Good point, moving to "fallback".
Later.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #508
And some other suggested changes for readability.