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 decorate grob vp widths for titles and footers when right margin is present #1245

Merged
merged 7 commits into from
May 24, 2024

Conversation

Melkiades
Copy link
Contributor

@Melkiades Melkiades added bug Something isn't working sme labels May 15, 2024
Copy link
Contributor

github-actions bot commented May 15, 2024

CLA Assistant Lite bot ✅ All contributors have signed the CLA

@Melkiades
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@Melkiades
Copy link
Contributor Author

recheck

Copy link
Contributor

github-actions bot commented May 15, 2024

Unit Tests Summary

    1 files     83 suites   1m 4s ⏱️
  838 tests   826 ✅  12 💤 0 ❌
1 803 runs  1 130 ✅ 673 💤 0 ❌

Results for commit edafc7a.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented May 15, 2024

badge

Code Coverage Summary

Filename                                   Stmts    Miss  Cover    Missing
---------------------------------------  -------  ------  -------  ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/abnormal_by_baseline.R                      65       0  100.00%
R/abnormal_by_marked.R                        55       5  90.91%   78-82
R/abnormal_by_worst_grade_worsen.R           116       3  97.41%   242-244
R/abnormal_by_worst_grade.R                   60       0  100.00%
R/abnormal.R                                  43       0  100.00%
R/analyze_variables.R                        162       3  98.15%   488, 512, 628
R/analyze_vars_in_cols.R                     176      33  81.25%   179, 202-207, 222, 236-237, 245-253, 259-265, 344-350
R/bland_altman.R                              92       1  98.91%   43
R/combination_function.R                       9       0  100.00%
R/compare_variables.R                         84       5  94.05%   130-134, 246, 305
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                          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, 238, 253, 261, 267-268
R/d_pkparam.R                                406       0  100.00%
R/decorate_grob.R                            113       0  100.00%
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, 481, 587
R/fit_rsp_step.R                              36       0  100.00%
R/fit_survival_step.R                         36       0  100.00%
R/formatting_functions.R                     183       2  98.91%   143, 278
R/g_forest.R                                 585      60  89.74%   241, 253-256, 261-262, 276, 278, 288-291, 336-339, 346, 415, 502, 515, 519-520, 525-526, 539, 555, 602, 633, 708, 717, 723, 742, 797-817, 820, 831, 850, 905, 908, 1043-1048
R/g_ipp.R                                    133       0  100.00%
R/g_km.R                                     350      57  83.71%   286-289, 308-310, 364-367, 401, 429, 433-476, 483-487
R/g_lineplot.R                               238      28  88.24%   183, 196, 232, 272, 353-360, 383-384, 395-405, 497, 503, 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_km.R                                     509      41  91.94%   137, 189-194, 287, 378, 380-381, 392-394, 413, 420-421, 423-425, 433-435, 460, 465-468, 651-654, 1108-1119
R/h_logistic_regression.R                    468       3  99.36%   203-204, 273
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           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                        64       1  98.44%   89
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/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                         106       2  98.11%   174, 179
R/summarize_change.R                          30       0  100.00%
R/summarize_colvars.R                         10       0  100.00%
R/summarize_coxreg.R                         172       2  98.84%   203, 430
R/summarize_glm_count.R                      195      27  86.15%   206, 224-256, 301-302
R/summarize_num_patients.R                    93       5  94.62%   108-110, 244-245
R/summarize_patients_exposure_in_cols.R       96       1  98.96%   42
R/survival_biomarkers_subgroups.R             78       6  92.31%   113-118
R/survival_coxph_pairwise.R                   79      11  86.08%   45-46, 58-66
R/survival_duration_subgroups.R              197       6  96.95%   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       116       1  99.14%   72
R/utils_factor.R                             109       2  98.17%   84, 302
R/utils_ggplot.R                             110       0  100.00%
R/utils_grid.R                               126       5  96.03%   164, 279-286
R/utils_rtables.R                            100       9  91.00%   39, 46, 51, 58-62, 403-404
R/utils_split_funs.R                          52       2  96.15%   82, 94
R/utils.R                                    141       7  95.04%   118, 121, 124, 128, 137-138, 332
TOTAL                                      10405     556  94.66%

Diff against main

Filename             Stmts    Miss  Cover
-----------------  -------  ------  --------
R/decorate_grob.R       +1       0  +100.00%
TOTAL                   +1       0  +0.00%

Results for commit: edafc7a

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@Melkiades Melkiades requested a review from pawelru May 16, 2024 08:43
@Melkiades
Copy link
Contributor Author

Can someone please help me review this? @ayogasekaram @edelarua Thanks!!

@ayogasekaram ayogasekaram self-requested a review May 21, 2024 15:14
Copy link
Contributor

@edelarua edelarua 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 Davide!!

@pawelru
Copy link
Contributor

pawelru commented May 22, 2024

Hey Davide. I tried with the example from the issue but I still can't make it work properly. width_titles = grid::unit(1, "npc") should indicate full width without margins but apparently it's not like that...
image

Tested in RStudio for now. For some reason I struggle to test in VSCode but I will look into this next.

@pawelru
Copy link
Contributor

pawelru commented May 22, 2024

On VSCode it's ok... Let me dig more into this:
image

@Melkiades
Copy link
Contributor Author

@pawelru rstudio does not work superwell with viewport as far as I could see. Everything works for base R on my machine. Have not tried VS but my output in rstudio is identical to vs

@pawelru
Copy link
Contributor

pawelru commented May 22, 2024

You are right. Base R should be the main testing environment. Tested it there and it's good. Also tested your feature branch versus the main and I can notice that the issue is fixed now. I think it's good to go.
One question though. Is this already covered by any test? I can see edits in one test but it's about margins - not the width of tittle and/or footnote. If not - let's add a new one.

@Melkiades
Copy link
Contributor Author

You are right. Base R should be the main testing environment. Tested it there and it's good. Also tested your feature branch versus the main and I can notice that the issue is fixed now. I think it's good to go. One question though. Is this already covered by any test? I can see edits in one test but it's about margins - not the width of tittle and/or footnote. If not - let's add a new one.

It is a valid point; I could not think of a proper test for it. I can use the width of the viewport as a test. It is what needs to be respected for the wrapping at the end.

Btw the wrapping core is not keeping spaces and manual \n as far as I can see. We might want to use the one I wrote for {formatters} that keeps those. What do you think?

@pawelru
Copy link
Contributor

pawelru commented May 22, 2024

Btw the wrapping core is not keeping spaces and manual \n as far as I can see. We might want to use the one I wrote for {formatters} that keeps those. What do you think?

This I don't know to be honest. I'm afraid that I'm not that deep in this to make a statement.

I'm going to approve as I can see that the issue had been resolved. Please make a call about the test. If it can't be done then I guess we need to live with that.

@Melkiades Melkiades merged commit cdd4b87 into main May 24, 2024
28 of 29 checks passed
@Melkiades Melkiades deleted the 1240_fix_decorate_grob_vp_widths@main branch May 24, 2024 16:12
@github-actions github-actions bot locked and limited conversation to collaborators May 24, 2024
@Melkiades
Copy link
Contributor Author

Btw the wrapping core is not keeping spaces and manual \n as far as I can see. We might want to use the one I wrote for {formatters} that keeps those. What do you think?

This I don't know to be honest. I'm afraid that I'm not that deep in this to make a statement.

I'm going to approve as I can see that the issue had been resolved. Please make a call about the test. If it can't be done then I guess we need to live with that.

I added and closed an issue to track down my research. Testing for this issue can be a bit difficult considering the inconsistencies between different viewport page sizes' conversions

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working sme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants