-
Notifications
You must be signed in to change notification settings - Fork 3
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
Documentation updates. #93
Conversation
@@ -243,7 +243,7 @@ require_parent_group <- function( | |||
#' Primary compressor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a ton of clean up in here. Nothing should have changed but the duplication is basically all gone. A couple check issues are taken care of here too.
``` | ||
|
||
By default, reads and writes are performed sequentially (i.e., not in parallel). | ||
Users can opt-in to parallel read/write functionality via `options`. | ||
|
||
```{r} | ||
```{r, eval=FALSE} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern of installing is a little dubious, at least it shouldn't run when the vignette is run start to finish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we replace the install.packages
with a stop()
failure that suggests installation of the package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. stop with a message saying the package should be installed is good, or if in interactive mode, it could prompt for user input?
expect_equal(bench_df$total_time[[1]] > bench_df$total_time[[2]], TRUE) | ||
expect_equal(bench_df$total_time[[2]] > bench_df$total_time[[3]], TRUE) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to speed these tests up and also make them cran compatible.
Alright -- that's it from me for a bit. Need to turn to some other things next week but can keep up with messages if you have some time to push this ahead. |
@keller-mark -- do you want to merge this in and start a final push for CRAN soon? |
@keller-mark -- been a while on this. I see some progress has been made in general. I could probably do another push on this project if you have any time too? |
There is a lot of interest in the R community for this, so if you guys wanna sprint for documentation and CRAN submission, I am up for it! |
@keller-mark and I have met in a workshop this week and discussed this. There is also a PR for a bioinformatics related project scverse/anndataR#190 that will depend on pizzarr so it has to go to a public repo soon. Shall we speed up stuff to do for CRAN? @dblodgett-usgs I can also push documentation related changes to your dev if you need further help. |
I've been progressing some dependent work as well. https://github.com/DOI-USGS/rnz and hypertidy/ncmeta#52 If we can get this PR merged and step back for a more holistic review of the project, I think it is more or less ready to be submitted to CRAN. |
awesome, @keller-mark lets do this! |
Merge branch 'dev' # Conflicts: # R/zarr-array.R
The latest addition here fixes #115 |
tests/testthat/test-01-parallel.R
Outdated
@@ -5,7 +5,7 @@ SlowGettingDirectoryStore <- R6::R6Class("SlowGettingDirectoryStore", | |||
public = list( | |||
get_item = function(key) { | |||
# Simulate a slow read such as an HTTP request. | |||
Sys.sleep(1.0/10.0) | |||
Sys.sleep(1.0/5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes the sleep duration longer, were you intending to make it shorter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was having a hard time getting the parallel version to be consistently faster due to per process overhead I think.
No description provided.