-
Notifications
You must be signed in to change notification settings - Fork 1
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
Features/42 read asc perkin elmer #47
Conversation
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 added comments before
R/read_asc_PerkinElmer.R
Outdated
for(flu in flu_file){ | ||
expect_message(spc <- read_asc_PerkinElmer(flu)) | ||
|
||
expect_equal(length(spc@wavelength), 181) |
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.
Checking the length only is fine with me - but if we check the colnames of $spc
, it is inconsistent not to check the actual wavelengths.
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.
The values of the lowest, the highest, and a few internediate wavelengths should be checked in addition to the length of this vector.
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.
The change looks fine, but the checks fail due to a missing function? Is that function in the develop
branch already, so if we merge, it will be fine? If so, consider this approved and merge it at your convenience. Otherwise please figure out where that renamed function is, I've seen it discussed today but don't recall where.
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.
@sangttruong, add unit tests defined in template #55 as additional tests to the current ones.
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.
Looks like all requested changes have been addressed. Go ahead and merge it.
Add unit test for read_asc_PerkinElmer.R
Resolve #42