search

Merck / metalite.ae

An R package for standard adverse events analysis
http://merck.github.io/metalite.ae/
GNU General Public License v3.0
17 stars 4 forks source link

fix bug for check population and observation grouping NA #137

Closed wangben718 closed 1 year ago

wangben718 commented 1 year ago

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.

BrianLang commented 1 year ago

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?

wangben718 commented 1 year ago

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?

BrianLang commented 1 year ago

I think the checks got lost due to the automated pushing of the styler updates (which don't trigger checks) https://github.com/Merck/metalite.ae/actions/runs/4508311581. has the test failures, and the artifacts which can be used to check what is different between new / old snapshots.

wangben718 commented 1 year ago

I think the checks got lost due to the automated pushing of the styler updates (which don't trigger checks) https://github.com/Merck/metalite.ae/actions/runs/4508311581. has the test failures, and the artifacts which can be used to check what is different between new / old snapshots.

Got it! Thanks for your clarification.

BrianLang commented 1 year ago

I think the checks got lost due to the automated pushing of the styler updates (which don't trigger checks) https://github.com/Merck/metalite.ae/actions/runs/4508311581. has the test failures, and the artifacts which can be used to check what is different between new / old snapshots.

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. From your branch you can always grab the version of the snaps which are on the master branch by calling this while on your dev branch: git checkout main -- tests/testthat/_snaps/<file name>

wangben718 commented 1 year ago

a short term fix is to revert back the snaps in your branch to those on the master branch. From your branch you can always grab the version of the snaps which are on the master branch by calling this while on your dev 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?

BrianLang commented 1 year ago

a short term fix is to revert back the snaps in your branch to those on the master branch. From your branch you can always grab the version of the snaps which are on the master branch by calling this while on your dev 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...

nanxstats commented 1 year ago

OK, I re-generated (reverted) the RTF snapshots using the CRAN version of r2rtf.

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?

@BrianLang Yes, that is exactly what I was saying. More accurately, it's about not using the "most current version from CRAN".

I tried to re-install r2rtf both from CRAN and github. I have got the same RTF output as I pushed.

@wangben718 Maybe you think you are using it, but you are not actually using it. Double check where you are loading your packages from.