Skip to content
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

fix format precision for format_count_fraction_fixed_dp #1192

Merged
merged 10 commits into from
Feb 26, 2024

Conversation

Melkiades
Copy link
Contributor

Fixes #1191

@Melkiades Melkiades added the sme label Feb 20, 2024
Copy link
Contributor

@ayogasekaram ayogasekaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Thanks @Melkiades

Copy link
Contributor

@shajoezhu shajoezhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @Melkiades , can you update the pr, cicd pipeline is failing, cheers

@Melkiades
Copy link
Contributor Author

I know it is silly to wonder, but I am wondering if it makes sense and if it is not too complicated to do restyling and roxygen checks one after the other so to have 2 automatic commits instead of having to do the second step manually. @cicdguy it is more difficult to explain than what you see in the commits (silly me I know)

Copy link
Contributor

github-actions bot commented Feb 21, 2024

badge

Code Coverage Summary

Filename                                   Stmts    Miss  Cover    Missing
---------------------------------------  -------  ------  -------  ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/abnormal_by_baseline.R                      68       2  97.06%   78-79
R/abnormal_by_marked.R                        55       5  90.91%   78-82
R/abnormal_by_worst_grade_worsen.R           116       3  97.41%   240-242
R/abnormal_by_worst_grade.R                   60       0  100.00%
R/abnormal.R                                  43       0  100.00%
R/analyze_variables.R                        190      10  94.74%   489-490, 506, 530, 686-687, 692-693, 711-712
R/analyze_vars_in_cols.R                     179      35  80.45%   168-169, 184, 207-212, 227, 241-242, 250-258, 264-270, 349-355
R/bland_altman.R                              92      55  40.22%   37, 78-133
R/combination_function.R                       9       0  100.00%
R/compare_variables.R                        124      17  86.29%   131-135, 247, 325-334, 389-390, 396
R/control_incidence_rate.R                    20       8  60.00%   32-35, 38-41
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                          50       1  98.00%   67
R/count_missed_doses.R                        34       0  100.00%
R/count_occurrences_by_grade.R               113       5  95.58%   101, 151-153, 156
R/count_occurrences.R                        115       1  99.13%   108
R/count_patients_events_in_cols.R             67       1  98.51%   53
R/count_patients_with_event.R                 47       0  100.00%
R/count_patients_with_flags.R                 58       4  93.10%   56-57, 62-63
R/count_values.R                              27       0  100.00%
R/cox_regression_inter.R                     154       0  100.00%
R/cox_regression.R                           161       0  100.00%
R/coxph.R                                    167       7  95.81%   191-195, 239, 254, 262, 268-269
R/d_pkparam.R                                406       0  100.00%
R/decorate_grob.R                            173      40  76.88%   235-266, 326-328, 339, 360-397
R/desctools_binom_diff.R                     621      64  89.69%   53, 88-89, 125-126, 129, 199, 223-232, 264, 266, 286, 290, 294, 298, 353, 356, 359, 362, 422, 430, 439, 444-447, 454, 457, 466, 469, 516-517, 519-520, 522-523, 525-526, 593, 604-616, 620, 663, 676, 680
R/df_explicit_na.R                            30       0  100.00%
R/estimate_multinomial_rsp.R                  50       1  98.00%   63
R/estimate_proportion.R                      205      12  94.15%   78-85, 89, 94, 315, 482, 588
R/fit_rsp_step.R                              36       0  100.00%
R/fit_survival_step.R                         36       0  100.00%
R/formatting_functions.R                     181       2  98.90%   145, 280
R/g_forest.R                                 569     412  27.59%   183-186, 189-192, 195-201, 204-207, 210-213, 240, 252-255, 260-261, 277, 287-290, 335-338, 345, 414, 491-1011
R/g_lineplot.R                               206      34  83.50%   168, 181, 210, 236-239, 315-322, 340-341, 347-357, 449, 455, 457, 499-500, 504-505
R/g_step.R                                    68       1  98.53%   109
R/g_waterfall.R                               47       0  100.00%
R/h_adsl_adlb_merge_using_worst_flag.R        73       0  100.00%
R/h_biomarkers_subgroups.R                    45       0  100.00%
R/h_cox_regression.R                         110       0  100.00%
R/h_logistic_regression.R                    468       3  99.36%   206-207, 276
R/h_map_for_count_abnormal.R                  57       2  96.49%   77-78
R/h_pkparam_sort.R                            15       0  100.00%
R/h_response_biomarkers_subgroups.R           90      12  86.67%   50-55, 107-112
R/h_response_subgroups.R                     178      18  89.89%   257-270, 329-334
R/h_stack_by_baskets.R                        67       3  95.52%   68-69, 95
R/h_step.R                                   180       0  100.00%
R/h_survival_biomarkers_subgroups.R           88       6  93.18%   111-116
R/h_survival_duration_subgroups.R            207      18  91.30%   259-271, 336-341
R/imputation_rule.R                           17       2  88.24%   54-55
R/incidence_rate.R                            96       7  92.71%   44-51
R/individual_patient_plot.R                  133       0  100.00%
R/kaplan_meier_plot.R                        695      76  89.06%   236-239, 279-314, 323-327, 538, 682-683, 715, 725-727, 735-737, 762, 769-770, 943-946, 1169, 1363-1368, 1404, 1504-1515
R/logistic_regression.R                      102       0  100.00%
R/missing_data.R                              21       3  85.71%   32, 66, 76
R/odds_ratio.R                               109       0  100.00%
R/prop_diff_test.R                            91       0  100.00%
R/prop_diff.R                                265      16  93.96%   62-65, 97, 282-289, 432, 492, 597
R/prune_occurrences.R                         57      10  82.46%   138-142, 188-192
R/response_biomarkers_subgroups.R             68       6  91.18%   189-194
R/response_subgroups.R                       192      10  94.79%   95-100, 276, 324-326
R/riskdiff.R                                  59       7  88.14%   102-105, 114, 124-125
R/rtables_access.R                            38       4  89.47%   159-162
R/score_occurrences.R                         20       1  95.00%   124
R/split_cols_by_groups.R                      49       0  100.00%
R/stat.R                                      59       3  94.92%   73-74, 129
R/summarize_ancova.R                         104       2  98.08%   172, 177
R/summarize_change.R                          30       0  100.00%
R/summarize_colvars.R                         13       2  84.62%   72-73
R/summarize_coxreg.R                         178       6  96.63%   201-202, 209, 346-347, 442
R/summarize_glm_count.R                      195      27  86.15%   206, 224-256, 301-302
R/summarize_num_patients.R                    99       9  90.91%   108-110, 160-161, 252-257
R/summarize_patients_exposure_in_cols.R       96       1  98.96%   42
R/survival_biomarkers_subgroups.R             70       6  91.43%   112-117
R/survival_coxph_pairwise.R                   79      11  86.08%   45-46, 58-66
R/survival_duration_subgroups.R              191       6  96.86%   119-124
R/survival_time.R                             79       0  100.00%
R/survival_timepoint.R                       113       7  93.81%   120-126
R/utils_checkmate.R                           68       0  100.00%
R/utils_default_stats_formats_labels.R       136       4  97.06%   72, 577-580
R/utils_factor.R                             109       2  98.17%   84, 302
R/utils_ggplot.R                              72       0  100.00%
R/utils_grid.R                               111       5  95.50%   149, 258-265
R/utils_rtables.R                            100       9  91.00%   39, 46, 51, 58-62, 403-404
R/utils_split_funs.R                          52       2  96.15%   81, 93
R/utils.R                                    141      10  92.91%   92, 94, 98, 118, 121, 124, 128, 137-138, 332
TOTAL                                      10307    1037  89.94%

Diff against main

Filename                    Stmts    Miss  Cover
------------------------  -------  ------  -------
R/formatting_functions.R        0      -1  +0.55%
R/utils.R                      +4       0  +0.21%
TOTAL                          +4      -1  +0.01%

Results for commit: 7cff5e6

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Feb 21, 2024

Unit Tests Summary

    1 files     83 suites   1m 5s ⏱️
  821 tests   794 ✅  27 💤 0 ❌
1 735 runs  1 075 ✅ 660 💤 0 ❌

Results for commit 7cff5e6.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Feb 21, 2024

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
formats 👶 $+0.07$ formats_with_nominator_to_denominator_are_always_formatted_as_1

Results for commit 118c0a4

♻️ This comment has been updated with latest results.

@cicdguy
Copy link
Contributor

cicdguy commented Feb 21, 2024

I know it is silly to wonder, but I am wondering if it makes sense and if it is not too complicated to do restyling and roxygen checks one after the other so to have 2 automatic commits instead of having to do the second step manually. @cicdguy it is more difficult to explain than what you see in the commits (silly me I know)

Totally understand that the autocommits are painful.

Technically, yes, it is possible to run one workflow after the other, but I would rather have them run simultaneously and report the inconsistencies so you, as a developer, can take the action to correct one or both (or more) issues in a single commit. I care for fast developer feedback, and that is accomplished with parallel workflows. Running them sequentially is not great from a performance and quick DevEx perspective. WDYT?

@Melkiades
Copy link
Contributor Author

I know it is silly to wonder, but I am wondering if it makes sense and if it is not too complicated to do restyling and roxygen checks one after the other so to have 2 automatic commits instead of having to do the second step manually. @cicdguy it is more difficult to explain than what you see in the commits (silly me I know)

Totally understand that the autocommits are painful.

Technically, yes, it is possible to run one workflow after the other, but I would rather have them run simultaneously and report the inconsistencies so you, as a developer, can take the action to correct one or both (or more) issues in a single commit. I care for fast developer feedback, and that is accomplished with parallel workflows. Running them sequentially is not great from a performance and quick DevEx perspective. WDYT?

You are right, we would lose speed and we do not want that! Thanks for the feedback ;)

@Melkiades Melkiades enabled auto-merge (squash) February 26, 2024 14:59
@Melkiades Melkiades merged commit b98b676 into main Feb 26, 2024
24 checks passed
@Melkiades Melkiades deleted the 1191_fix_format_precision@main branch February 26, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: format_count_fraction_fixed_dp
4 participants