Replace deprecated `forcats::fct_explicit_na` #817

edelarua · 2023-02-07T14:24:45Z

Closes #811

tests/testthat/_snaps/summarize_num_patients.md

NEWS.md

Melkiades · 2023-02-07T17:27:42Z

@cicdguy, we think the Docker image is missing the most recent version of forcats that is needed for this. Can we have it at 1.0.0 ? Also, we were wondering how to check grammar locally, so we are faster at writing texts

edelarua · 2023-02-07T17:32:24Z

@cicdguy, we think the Docker image is missing the most recent version of forcats that is needed for this. Can we have it at 1.0.0 ?

@Melkiades Arek is rebuilding the ci-image to resolve the forcats version issue.

github-actions · 2023-02-07T21:32:03Z

Unit Tests Summary

      1 files   77 suites 1m 41s ⏱️
  718 tests 718 ✔️     0 💤 0 ❌
1 527 runs 960 ✔️ 567 💤 0 ❌

Results for commit f69cae8.

♻️ This comment has been updated with latest results.

github-actions · 2023-02-07T21:32:08Z

Unit Test Performance Difference

Test Suite	$Status$	Time on `main`	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
abnormal_by_marked	💚	$46.10$	$-1.24$	$0$	$0$	$0$	$0$
g_lineplot	💀	$0.60$	$-0.60$	$-2$	$0$	$-2$	$0$
h_adsl_adlb_merge_using_worst_flag	💚	$2.62$	$-2.22$	$-80$	$0$	$0$	$0$
h_stack_by_baskets	💚	$4.59$	$-4.37$	$-133$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on `main`	$±Time$	Test Case
g_lineplot	💀	$0.45$	$-0.45$	g_lineplot_works_with_custom_settings_and_statistics_table
g_lineplot	💀	$0.14$	$-0.14$	g_lineplot_works_with_default_settings
h_stack_by_baskets	💚	$1.09$	$-1.01$	h_stack_by_baskets_returns_the_correct_dataframe

Results for commit 72efcc6

♻️ This comment has been updated with latest results.

edelarua · 2023-02-07T21:57:25Z

@Melkiades versioning is fixed but I'm not able to reproduce the g_lineplot warnings that are causing test failures. I'm also not sure where these errors would stem from since g_lineplot doesn't seem to use any of the functions that were touched in this PR. Can you reproduce the failed tests?

EDIT: Looks like these g_lineplot tests are also failing on main, so unrelated to this PR. I’ll see if I can reproduce the warnings tomorrow.

Melkiades · 2023-02-08T12:16:22Z

I can not reproduce the warnings locally. I am updating everything and see if something changes. Still nothing

Melkiades

Lgtm anyway @edelarua! Thank you 👍

github-actions · 2023-02-08T18:44:30Z

Code Coverage Summary

Filename                                   Stmts    Miss  Cover    Missing
---------------------------------------  -------  ------  -------  ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/abnormal_by_baseline.R                      63       0  100.00%
R/abnormal_by_marked.R                        52       5  90.38%   124-128
R/abnormal_by_worst_grade_worsen.R           113       3  97.35%   234-236
R/abnormal_by_worst_grade.R                   37       0  100.00%
R/abnormal.R                                  40       0  100.00%
R/analyze_vars_in_cols.R                      37       1  97.30%   114
R/combination_function.R                       9       0  100.00%
R/compare_variables.R                        138       3  97.83%   133, 243, 260
R/control_incidence_rate.R                    10       0  100.00%
R/control_logistic.R                           7       0  100.00%
R/control_step.R                              23       1  95.65%   58
R/control_survival.R                          15       0  100.00%
R/count_cumulative.R                          47       1  97.87%   60
R/count_missed_doses.R                        31       0  100.00%
R/count_occurrences_by_grade.R                84       6  92.86%   152-154, 157, 172-173
R/count_occurrences.R                         61       1  98.36%   89
R/count_patients_events_in_cols.R             67       1  98.51%   72
R/count_patients_with_event.R                 72       0  100.00%
R/count_values.R                              24       0  100.00%
R/cox_regression_inter.R                     142       6  95.77%   92, 172-176
R/cox_regression.R                           208      11  94.71%   367-375, 394-396
R/coxph.R                                    168       7  95.83%   224-228, 272, 287, 295, 301-302
R/d_pkparam.R                                405       0  100.00%
R/decorate_grob.R                            167       6  96.41%   269-275, 382, 414, 424, 431
R/desctools_binom_diff.R                     668      68  89.82%   65, 100-101, 141-142, 145, 224, 250-261, 300, 302, 322, 326, 330, 334, 386, 389, 392, 395, 456, 464, 476-477, 483-486, 494, 497, 506, 509, 557-558, 560-561, 563-564, 566-567, 640, 652-665, 670, 717, 730, 734
R/df_explicit_na.R                            30       0  100.00%
R/estimate_multinomial_rsp.R                  47       1  97.87%   56
R/estimate_proportion.R                      198      11  94.44%   74-81, 85, 90, 458, 565
R/fit_rsp_step.R                              36       0  100.00%
R/fit_survival_step.R                         36       0  100.00%
R/footnotes.R                                  5       0  100.00%
R/formats.R                                  115       3  97.39%   101, 138, 148
R/g_forest.R                                 441      25  94.33%   199, 251-252, 256-257, 324, 341-342, 347-348, 361, 377, 424, 455, 531, 540, 621-625, 635, 703, 706, 830
R/g_lineplot.R                               199     180  9.55%    162-418, 462, 468, 470, 508-515
R/g_step.R                                    68       1  98.53%   107
R/g_waterfall.R                               47       0  100.00%
R/h_adsl_adlb_merge_using_worst_flag.R        74       0  100.00%
R/h_biomarkers_subgroups.R                    38       0  100.00%
R/h_cox_regression.R                         110       0  100.00%
R/h_logistic_regression.R                    468       3  99.36%   197-198, 265
R/h_map_for_count_abnormal.R                  54       0  100.00%
R/h_pkparam_sort.R                            15       0  100.00%
R/h_response_biomarkers_subgroups.R           74       0  100.00%
R/h_response_subgroups.R                     171      12  92.98%   244-257
R/h_stack_by_baskets.R                        65       1  98.46%   94
R/h_step.R                                   180       0  100.00%
R/h_survival_biomarkers_subgroups.R           78       0  100.00%
R/h_survival_duration_subgroups.R            200      12  94.00%   250-262
R/incidence_rate.R                            93       7  92.47%   68-75
R/individual_patient_plot.R                  133       0  100.00%
R/kaplan_meier_plot.R                        532      29  94.55%   256-260, 455, 622-624, 632-634, 660, 667-668, 839, 1020, 1258-1269
R/logistic_regression.R                      101       0  100.00%
R/missing_data.R                              21       3  85.71%   30, 61, 71
R/odds_ratio.R                               106       0  100.00%
R/prop_diff_test.R                            87       0  100.00%
R/prop_diff.R                                255      12  95.29%   95, 250-257, 398, 461, 572
R/prune_occurrences.R                         57      10  82.46%   130-134, 174-178
R/response_biomarkers_subgroups.R             59       0  100.00%
R/response_subgroups.R                       164       4  97.56%   263, 305-307
R/rtables_access.R                            36       2  94.44%   143-144
R/score_occurrences.R                         20       1  95.00%   114
R/split_cols_by_groups.R                      49       0  100.00%
R/stat.R                                      47       3  93.62%   76-77, 132
R/summarize_ancova.R                          95       1  98.95%   187
R/summarize_change.R                          27       0  100.00%
R/summarize_colvars.R                          6       0  100.00%
R/summarize_glm_count.R                      164       4  97.56%   181, 186, 249, 318
R/summarize_num_patients.R                    67       5  92.54%   95-97, 189-190
R/summarize_patients_exposure_in_cols.R       46       0  100.00%
R/summarize_variables.R                      212      20  90.57%   85-102, 264, 467
R/survival_biomarkers_subgroups.R             59       0  100.00%
R/survival_coxph_pairwise.R                   73       9  87.67%   66-74
R/survival_duration_subgroups.R              171       0  100.00%
R/survival_time.R                             47       0  100.00%
R/survival_timepoint.R                       114       7  93.86%   145-151
R/utils_checkmate.R                           68       0  100.00%
R/utils_factor.R                              87       1  98.85%   93
R/utils_grid.R                               111       5  95.50%   148, 257-263
R/utils_rtables.R                             84       7  91.67%   25, 32-36, 344-345
R/utils.R                                    137      10  92.70%   100, 102, 106, 126, 129, 132, 136, 145-146, 332
R/wrap_text.R                                 66       5  92.42%   38, 59, 79, 86, 108
TOTAL                                       8831     514  94.18%

Diff against main

Filename                   Stmts    Miss  Cover
-----------------------  -------  ------  -------
R/g_lineplot.R                 0    +151  -75.88%
R/missing_data.R              +1       0  +0.71%
R/summarize_variables.R        0     +18  -8.49%
R/utils_factor.R              +1       0  +0.01%
TOTAL                         +2    +169  -1.91%

Results for commit: 72efcc6

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

edelarua · 2023-02-08T20:34:31Z

@Melkiades I was not able to reproduce either. We could remove these tests for now (they were recently added) and look into this later.

EDIT: I have created a new issue to investigate the failing tests (#820).

edelarua · 2023-02-08T21:08:57Z

@shajoezhu This PR will fix tern for now and unblock issues in downstream dependencies (including chevron which is currently failing because of this deprecated function), but I see that you have added the blocked label. Should we still hold off on merging this?

shajoezhu · 2023-02-08T23:40:21Z

Hi @edelarua , yes please. Let's block this for one more day. I will follow up in the office with Liming, and we test it.

clarkliming · 2023-02-09T08:37:33Z

works fine for downstread side 👍

shajoezhu · 2023-02-09T08:45:49Z

awesome! We are good to go! @Melkiades @edelarua

edelarua added 3 commits February 1, 2023 19:26

Replace deprecated forcats function

e5e3315

Merge branch 'main' into 811_fct_explicit_na@main

dac8eac

Fix tests

08251c7

edelarua added the sme label Feb 7, 2023

edelarua marked this pull request as ready for review February 7, 2023 14:24

Melkiades reviewed Feb 7, 2023

View reviewed changes

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

Melkiades reviewed Feb 7, 2023

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

edelarua added 3 commits February 7, 2023 11:02

Fix grammar

ec312fb

Fix grammar

2d19337

Fix grammar

b9d9c1b

empty commit

7101c5d

Merge branch 'main' into 811_fct_explicit_na@main

139ca2f

Melkiades approved these changes Feb 8, 2023

View reviewed changes

shajoezhu added the blocked label Feb 8, 2023

test warning output

8ac9484

Revert testing

8cd7a1c

Remove broken tests

f69cae8

edelarua mentioned this pull request Feb 8, 2023

Investigate warnings from g_lineplot #820

Closed

insightsengineering deleted a comment from shajoezhu Feb 9, 2023

shajoezhu removed the blocked label Feb 9, 2023

Melkiades merged commit 681e57a into main Feb 9, 2023

Melkiades deleted the 811_fct_explicit_na@main branch February 9, 2023 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace deprecated `forcats::fct_explicit_na` #817

Replace deprecated `forcats::fct_explicit_na` #817

edelarua commented Feb 7, 2023

Melkiades commented Feb 7, 2023

edelarua commented Feb 7, 2023

github-actions bot commented Feb 7, 2023 •

edited

Loading

github-actions bot commented Feb 7, 2023 •

edited

Loading

edelarua commented Feb 7, 2023 •

edited

Loading

Melkiades commented Feb 8, 2023 •

edited

Loading

Melkiades left a comment

github-actions bot commented Feb 8, 2023 •

edited

Loading

edelarua commented Feb 8, 2023 •

edited

Loading

edelarua commented Feb 8, 2023

shajoezhu commented Feb 8, 2023

clarkliming commented Feb 9, 2023

shajoezhu commented Feb 9, 2023

Replace deprecated forcats::fct_explicit_na #817

Replace deprecated forcats::fct_explicit_na #817

Conversation

edelarua commented Feb 7, 2023

Melkiades commented Feb 7, 2023

edelarua commented Feb 7, 2023

github-actions bot commented Feb 7, 2023 • edited Loading

Unit Tests Summary

github-actions bot commented Feb 7, 2023 • edited Loading

Unit Test Performance Difference

edelarua commented Feb 7, 2023 • edited Loading

Melkiades commented Feb 8, 2023 • edited Loading

Melkiades left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 8, 2023 • edited Loading

Code Coverage Summary

Diff against main

edelarua commented Feb 8, 2023 • edited Loading

edelarua commented Feb 8, 2023

shajoezhu commented Feb 8, 2023

clarkliming commented Feb 9, 2023

shajoezhu commented Feb 9, 2023

Replace deprecated `forcats::fct_explicit_na` #817

Replace deprecated `forcats::fct_explicit_na` #817

github-actions bot commented Feb 7, 2023 •

edited

Loading

github-actions bot commented Feb 7, 2023 •

edited

Loading

edelarua commented Feb 7, 2023 •

edited

Loading

Melkiades commented Feb 8, 2023 •

edited

Loading

github-actions bot commented Feb 8, 2023 •

edited

Loading

edelarua commented Feb 8, 2023 •

edited

Loading