fix bug for check population and observation grouping NA #137
Conversation
There was a problem hiding this comment.
I don't think the snapshots should be changed here - you should use the latest version of r2rtf on CRAN: Merck/r2rtf#137.
Also, I would recommend always filling out the description field of a pull request to provide sufficient context besides the title.
Are you suggesting that maybe @wangben718 is not using the most current version of r2rtf and this is why the snapshots are different on his local machine compared to what GitHub actions is using? |
|
I tried to re-install r2rtf both from CRAN and github. I have got the same RTF output as I pushed. One another thing, I saw an error for passing build check but I could not find the details under the 'Checks' tab. Is there anything changed in our package? |
|
I think the checks got lost due to the automated pushing of the styler updates (which don't trigger checks) |
Got it! Thanks for your clarification. |
It's not actually clear to me that the artifacts uploaded give the new snapshots... Assuming Nan is right and there is no reason for the snaps to have been updated, a short term fix is to revert back the snaps in your branch to those on the master branch. |
Yes, we can revert back. Actually it is not the first time I got an unexpected updated snapshot from function related to r2rtf. Is it probably beacuse the original snapshot are using a old version r2rtf? |
yeah it's also not clear to me what's going on, but I'm currently experiencing this with our mkhecon dev work and disparities between Jenkins and local snapshots of rtfs... |
|
OK, I re-generated (reverted) the RTF snapshots using the CRAN version of r2rtf.
@BrianLang Yes, that is exactly what I was saying. More accurately, it's about not using the "most current version from CRAN".
@wangben718 Maybe you think you are using it, but you are not actually using it. Double check where you are loading your packages from. |
R/prepare_ae_specific.R
Outdated
| @@ -93,6 +67,31 @@ prepare_ae_specific <- function(meta, | |||
| pop_group <- collect_adam_mapping(meta, population)$group | |||
| obs_group <- collect_adam_mapping(meta, observation)$group | |||
|
|
|||
| # Check if the grouping variable is missing | |||
| # pop_grp <- vapply(meta$population, "[[", FUN.VALUE = character(1), "group") | |||
There was a problem hiding this comment.
Please don't have commented out code.
There was a problem hiding this comment.
Please don't have commented out code.
I haved removed the code. Thanks for review!
There was a problem hiding this comment.
Hi @nanxstats , since @wangben718 already removed the comments, I directly merged this PR to the master, considering there are two teammates who need the package for slide preparation early this week...
LittleBeannie
dismissed
nanxstats’s stale review
Hi @nanxstats , since @wangben718 already removed the comments, I directly merged this PR to the master, considering there are two teammates who need the package for slide preparation early this week...
When checking 'grouping' variable missing or not, we were using the input population/observation dataset. We should check the dataset after subset by population/observation term.