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.

Sign up for GitHub

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

Jump to bottom

Refactoring of `analyze_vars` with specifics for .stat_names #1361

Merged

shajoezhu merged 44 commits into main from 1352_fix_stats@main

Jan 16, 2025

Contributor

Melkiades commented Dec 11, 2024

Pull Request

Fixes #1352

Melkiades added 3 commits

December 10, 2024 12:48


          initial rework of analyze_variables to allow custom stats and better …

4c83153

…default handlings


          add of initial stats

c9b8a82


          Fix: docs

b647e66

Melkiades added enhancement sme labels

Melkiades marked this pull request as draft

December 11, 2024 14:14

github-actions bot and others added 8 commits

December 11, 2024 14:16


          [skip style] [skip vbump] Restyle files

b2b2969


          reducing unneeded complexity

c113b9e


          merge

b4e6920

up

0ac4cae


          fix multiple issues with logicals

14815f7


          fixing all errors and test

bf128e8


          fix docs

c343262


          minor fixes

783039a

Melkiades commented

View reviewed changes

tests/testthat/_snaps/analyze_variables.md Show resolved Hide resolved

Melkiades commented

View reviewed changes

tests/testthat/_snaps/analyze_variables.md Outdated Show resolved Hide resolved

Melkiades commented

View reviewed changes

tests/testthat/_snaps/analyze_variables.md Outdated Show resolved Hide resolved

Melkiades commented

View reviewed changes

tests/testthat/_snaps/analyze_variables.md Show resolved Hide resolved

Melkiades commented

View reviewed changes

R/analyze_variables.R Outdated Show resolved Hide resolved

Melkiades commented

View reviewed changes

R/analyze_variables.R Outdated Show resolved Hide resolved


          add: more tests

b2b2a5c

Melkiades marked this pull request as ready for review

December 16, 2024 16:00

Melkiades commented

View reviewed changes

tests/testthat/_snaps/analyze_variables.md Show resolved Hide resolved

Melkiades and others added 3 commits

December 20, 2024 00:11


          minor fixes

e1df39b


          [skip style] [skip vbump] Restyle files

1dd2019


          [skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

461ea9a

Melkiades commented

View reviewed changes

R/utils_default_stats_formats_labels.R Outdated Show resolved Hide resolved

Melkiades commented

View reviewed changes

R/utils_factor.R Show resolved Hide resolved

Melkiades commented

View reviewed changes

R/utils_factor.R Show resolved Hide resolved

Melkiades commented

View reviewed changes

tests/testthat/_snaps/analyze_variables.md Outdated Show resolved Hide resolved


          update doc

ef23cad

fix test

use as.facotr instead

update example

update example

save changes

shajoezhu force-pushed the 1352_fix_stats@main branch from 4cf2edf to ef23cad Compare

January 15, 2025 03:43

shajoezhu and others added 14 commits

January 15, 2025 12:18


          stash changes

f607e90


          [skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

35886b5


          update test case

628b4e4


          updatE

acebbaa


          change default value

010bd5e


          [skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

96e738b


          update

4aeb61c


          Merge branch '1352_fix_stats@main' of github.com:insightsengineering/…

231b959

…tern into 1352_fix_stats@main


          Merge remote-tracking branch 'origin/1352_fix_stats@main' into 1352_f…

8a2d986

…ix_stats@main


          Documentation improvements

707336b


          Update compare_vars documentation

f56fd96


          Fix inconsistencies

544769c


          Fix missing NAs

b9b4796


          Set seed in tests

071d884

Contributor

edelarua commented Jan 15, 2025

@shajoezhu I think all of the issues with this PR have been fixed. I'm not sure why but the Pkgdown Docs check always seems to take longer to update its package dependencies, which I think is why this check still isn't passing.

edelarua and others added 3 commits

January 15, 2025 18:43


          Add project id

d2e1ee3


          Merge branch '1352_fix_stats@main' of github.com:insightsengineering/…

d308f61

…tern into 1352_fix_stats@main


          update workflow

5f8bb27

shajoezhu enabled auto-merge (squash)

January 16, 2025 03:30

shajoezhu approved these changes

View reviewed changes

Contributor

shajoezhu left a comment

this is brilliant work! Thanks a lot @Melkiades @edelarua

shajoezhu requested a review from edelarua

January 16, 2025 03:31

shajoezhu dismissed edelarua’s stale review

January 16, 2025 03:32

this is now resolved

This was referenced Jan 16, 2025

to fix: ungroup_stats should be removed in favor of get_*_from_stats #1370

Closed

pval handling to be refactored #1371

Open

shajoezhu merged commit d048143 into main

30 checks passed

shajoezhu deleted the 1352_fix_stats@main branch

January 16, 2025 03:34

github-actions bot locked and limited conversation to collaborators

Melkiades commented

View reviewed changes

R/analyze_variables.R

@@ @@ -780,7 +780,7 @@ analyze_vars <- function(lyt, @@
                   var_labels = var_labels,
                   afun = a_summary,
                   na_str = na_str,
-                  inclNAs = na_rm,
+                  inclNAs = !na_rm,

Contributor Author

Melkiades Jan 20, 2025

eheh this silly mistake got on my nerves! nice catch

Contributor Author

Melkiades Jan 20, 2025

I noticed there are different ways to handle NAs in each statistical function. What should we do about this? Keep NAs until the last statistical level and then do separate things or take them out here? @edelarua @shajoezhu

Contributor

shajoezhu Jan 20, 2025

lets fix this in new version. agree

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement sme