read_vpts() #590
Merged
read_vpts() #590
Conversation
This file contains 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
Merged
Codecov Report
@@ Coverage Diff @@
## master #590 +/- ##
==========================================
+ Coverage 81.53% 82.09% +0.55%
==========================================
Files 59 61 +2
Lines 3537 3692 +155
==========================================
+ Hits 2884 3031 +147
- Misses 653 661 +8
|
|
Absolutely! |
… scope of this function
adokter
requested changes
Jul 7, 2023
This reverts commit a9c42a7.
adokter
approved these changes
Jul 7, 2023
| ) | ||
|
|
||
| # Check whether time series is regular | ||
| heights <- as.integer(unique(data[["height"]])) |
There was a problem hiding this comment.
why convert to integer? we may have situations where the height is a float
| # Throw error if nrows per height are not identical | ||
|
|
||
| assertthat::assert_that( | ||
| isFactor(dim(data)[1], length(unique(data$height))) > 0, |
There was a problem hiding this comment.
we use lowercase throughout and separating words by _, so this should be rename is_factor. may be confusing though because factor is a specific data type in R, so would use a different word
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.
This PR is incomplete, but contains the start of a new
read_vpts()function that supports both hdf5 vp files and VPTS CSV files (#551). It would be very useful if the latter was included in bioRad 0.7.read_vpts(files)function checks if all provided file extensions are the same and then uses aswitch()to read the data with an appropriate helper function: eitherread_vpts_csv()orread_vpts_hdf5().read_vpts_csv()contains new functionality, but relies heavily on frictionless to read local and remote csv, gzipped or not (all supported by underlyingreadrdependency) and compares the data with the VPTS CSV table schema. It provides good warning/errors.read_vpts_hdf5()makes use ofread_vpfiles()and thenbind_into_vpts().read_vpts()has been renamed toread_stdout(). It is not integrated in the newread_vpts()because 1) that read function requires a lot more metadata parameters (lat,lon,height) that I did not want to integrate in the newread_vpts()and 2) support for vol2bird stdout format will be deprecated in the future./tests/testthat/helper.Rhas been added to aid in the download and temporary storage of hdf5 and csv files required for testing. When tests are run, a temporary folder/tempis created and subsequently removed at the end of thetest-read_vpts.Rscript.TODO
read_vpts()functionread_vpts_hdf5()functionread_vpts_csv()functionread_vpts_csv()can return data as df (useful for the future VPTS objects as data.frame #568)read_vpts_csv()can return data as a vpts object. A start for this has been made, code can take inspiration fromread_stdout()read_vpts(). Necessary tests are listed, but not writtenread_stdout(). These were not provided yet, but are not required as part of this PRread_stdout()ifread_vpts()is used with parameterslat,lon, etc.Unfortunately, I don't have the time to finish this. @iskandari would have the opportunity to complete this functionality? Ping me if you have any questions.