521 documentation review before the release #530

m7pr · 2023-06-26T13:36:52Z

Based on specification from #521

I checked that this PR #485 replaced all teal.code::chunks_* with teal.code::eval_code or teal.code::new_qenv. This looks like backend functionality (not exposed to the user) so chunks were not exposed in examples, hence even though I reviewed examples, nothing was there to be checked. Vignettes, nor README nor DESCRIPTION never mentioned the usage of chunks so no need to update them with any deprecation notes.

Unsure if there is a bigger purpose of this task besides bumping teal.code in DESCRIPTION file to 0.3.0. Am I missing something @gogonzo ?

github-actions · 2023-06-26T14:11:00Z

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  ----------------------------------------------
R/tm_a_pca.R                    821     821  0.00%    73-1019
R/tm_a_regression.R             710     710  0.00%    93-887
R/tm_data_table.R               171     171  0.00%    59-282
R/tm_file_viewer.R              172     172  0.00%    39-241
R/tm_front_page.R               127     116  8.66%    64-206
R/tm_g_association.R            327     327  0.00%    90-476
R/tm_g_bivariate.R              652     492  24.54%   125-703, 751, 757, 761, 872, 889, 907, 927-949
R/tm_g_distribution.R          1025    1025  0.00%    111-1258
R/tm_g_response.R               346     346  0.00%    86-496
R/tm_g_scatterplot.R            716     716  0.00%    143-950
R/tm_g_scatterplotmatrix.R      278     259  6.83%    79-374, 428, 442
R/tm_missing_data.R            1034    1034  0.00%    59-1229
R/tm_outliers.R                 944     944  0.00%    77-1143
R/tm_t_crosstable.R             251     251  0.00%    82-371
R/tm_variable_browser.R         815     810  0.61%    62-1294
R/utils.R                       122     122  0.00%    74-352
R/zzz.R                           1       1  0.00%    2
TOTAL                          8512    8317  2.29%

Diff against main

Filename                Stmts    Miss  Cover
--------------------  -------  ------  --------
R/tm_g_scatterplot.R       +4      +4  +100.00%
TOTAL                      +4      +4  -0.00%

Results for commit: d260d7c

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2023-06-26T14:18:44Z

Unit Tests Summary

  1 files   5 suites 0s ⏱️
16 tests 16 ✔️ 0 💤 0 ❌
49 runs 49 ✔️ 0 💤 0 ❌

Results for commit ec8b4d6.

♻️ This comment has been updated with latest results.

gogonzo · 2023-06-27T06:32:31Z

backend functionality (not exposed to the user) so chunks were not exposed in examples, hence even though I reviewed examples, nothing was there to be checked. Vignettes, nor README nor DESCRIPTION never mentioned the usage of chunks so no need to update them with any deprecation notes.

If you reviewwed examples please make sure you actually run them in an interactive session as they are not covered by devtools::run_examples.

Unsure if there is a bigger purpose of this task besides bumping teal.code in DESCRIPTION file to 0.3.0. Am I missing something @gogonzo ?

Yes, change is massive. We changed argument datasets (FilteredData) to data (tdata - list of reactive data + code as attribute).

changes main vs last release https://github.com/insightsengineering/teal.modules.general/compare/2022_10_13...main?diff=split

m7pr · 2023-06-27T07:20:48Z

@gogonzo I did check that there was a lot of changes during this PR #485
However I run all examples manually as they have this interactive statement verification

if (interactive()) {
  shinyApp(app$ui, app$server)
}

Some needed extra packages to be installed, like install.packages(c('goftest', 'ggpmisc')) at tm_g_distribution.R or install.packages('ggExtra') at tm_g_scatterplot.R, or install.packages('sparkline') at tm_variable_browser.R but those are listed in Suggests, so all is proper.

chlebowa · 2023-06-27T07:32:39Z

Some needed extra packages to be installed, like install.packages(c('goftest', 'ggpmisc')) at tm_g_distribution.R or install.packages('ggExtra') at tm_g_scatterplot.R, or install.packages('sparkline') at tm_variable_browser.R but those are listed in Suggests, so all is proper.

Did you also add appropriate messages for installing those packages? Ones like this appear in several places in teal.modules.general.

if (!requireNamespace("rtables", quietly = TRUE)) {
  stop("Cannot load rtables - please install the package or restart your session.")
}

m7pr · 2023-06-27T08:39:36Z

@chlebowa yes, those were already pasted as warnings/error messages

Error in tm_variable_browser(label = "Variable browser", ggplot2_args = teal.widgets::ggplot2_args(labs = list(subtitle = "Plot generated by Variable Browser Module")), :
Cannot load sparkline - please install the package or restart your session.

Error in tm_g_scatterplot(label = "Scatterplot Choices", x = data_extract_spec(dataname = "ADSL", :
Cannot load package(s): ggExtra.
Install or restart your session.Cannot load package(s): colourpicker.
Install or restart your session.

Error in tm_g_distribution(dist_var = data_extract_spec(dataname = "iris", :
Cannot load package(s): goftest.
Install or restart your session.

Error in tm_g_distribution(dist_var = data_extract_spec(dataname = "iris", :
Cannot load package(s): ggpmisc.
Install or restart your session.Cannot load package(s): ggpp.
Install or restart your session.Cannot load package(s): goftest.
Install or restart your session.

The above are results of such assertions on the beginning of the call. Below example taken from tm_g_scatterplot

  logger::log_info("Initializing tm_g_scatterplot")

  extra_packages <- c("ggpmisc", "ggExtra", "colourpicker")
  missing_packages <- Filter(function(x) !requireNamespace(x, quietly = TRUE), extra_packages)
  if (length(missing_packages) > 0L) {
    stop(sprintf("Cannot load package(s): %s.\nInstall or restart your session.",
                 paste(missing_packages, sep = ", ")))
  }

gogonzo · 2023-06-27T09:29:21Z

Below example taken from tm_g_scatterplot

Since it is already there let's not change it, message is explicit and clear. I don't have an opinion yet how we should handle dependencies yet. This is something we'd need to discuss soon

chlebowa · 2023-06-27T09:50:25Z

paste(missing_packages, sep = ", ")))

Oh, good. Can you fix the message, though? This sep should be collapse. Or better yet, use toString.

m7pr · 2023-06-27T09:53:02Z

We could open a prompt, asking a person for an action in this case

On a missing dependencies:

Do nothing and exit
Allow installation and exit
Allow installation and move forward with the function execution

…on and tm_g_scatterplot

m7pr · 2023-06-27T09:57:56Z

Oh, good. Can you fix the message, though? This sep should be collapse. Or better yet, use toString.

@chlebowa yeah, toString is neat 5387684

chlebowa · 2023-06-27T10:05:32Z

We could open a prompt, asking a person for an action in this case

On a missing dependencies:

Do nothing and exit

Allow installation and exit

Allow installation and move forward with the function execution

It's not strictly missing, the packages are listed in Suggests for a reason (that eludes me). Let's keep it as is.

m7pr · 2023-06-27T10:11:26Z

Yeah I know they are listed in Suggests. The popular convention is that, if a package is only used in one function that is not the main function of the package and package can live without this function, then it's better to have the package in Suggests, so it's not installed by default when installing the initial package (so that installation time is shorter and there is less dependencies clutter). The dependent packages in Suggests will be installed if someone specify install.packages('initial package', dependencies = c("Depends", "Imports", "Suggests")). Some packages are only used in vignettes or examples that are not run and those also should live in Suggests.

The only question, that we could tackle on a separate discussion, is if we:

do not do anything with it
at least thrown a meaningful error for a person trying to use a function that is based on a Suggested package - this is what currently is happening
go one step beyond and also prepare a workaround that also allows to hit ENTER on a prompt so that missing packages get installed for the user and the function continues

chlebowa · 2023-06-27T10:47:12Z

Yeah I know they are listed in Suggests. The popular convention is that, if a package is only used in one function that is not the main function of the package and package can live without this function, then it's better to have the package in Suggests, so it's not installed by default when installing the initial package (so that installation time is shorter and there is less dependencies clutter). The dependent packages in Suggests will be installed if someone specify install.packages('initial package', dependencies = c("Depends", "Imports", "Suggests")). Some packages are only used in vignettes or examples that are not run and those also should live in Suggests.

That is correct. The packages that we Suggest are most often ones we only use for examples.

The only question, that we could tackle on a separate discussion, is if we:

do not do anything with it

And that is what we do when they are encountered. Just raise an error.

m7pr · 2023-06-27T11:01:55Z

alrighty, getting back to tmg review

examples are run - no teal.code::chunks_*
code is reviewed - no teal.code::chunks_*
vignettes are checked - - teal.code::chunks_*
no narration anywhere about teal.code::chunks_* nor teal.code::qenv

I think it was a great job already made on this huge PR that introduced qenv

…sightsengineering/teal.modules.general into 521_review_documentation@main

gogonzo

looks good. Thanks for updating teal.code depedency

m7pr · 2023-06-28T13:49:16Z

Hey @gogonzo @pawelru the check for this PR does not pass because our own builds do not allow specific R CMD Check NOTES. In this case we do not allow any NOTES related to "checking examples .* NOTE" message https://github.com/insightsengineering/teal.modules.general/actions/runs/5401394235/jobs/9811839374?pr=530#step:36:50

In our case this is raised because examples take more than 5 seconds - they actually run for 8 seconds
https://github.com/insightsengineering/teal.modules.general/actions/runs/5401394235/jobs/9811839374?pr=530#step:20:114

Any idea what to do? Should we delete examples, or should we reduce NOTES check on GitHub Action checks?

gogonzo · 2023-06-28T14:09:02Z

@m7pr Please remove scda from dependency and make examples to use iris, mtcars - they would run much faster then.

Here is the issue (it's a part of our increment goal)
#532

m7pr · 2023-06-29T07:00:37Z

sure @gogonzo , will do! thanks

pawelru · 2023-06-29T07:57:43Z

Any idea what to do? Should we delete examples, or should we reduce NOTES check on GitHub Action checks?

I really don't know. I would like to still keep this note in our block-list if possible, i.e. I agree with @gogonzo to think how to optimize those examples. This is generally not good that example takes long time to execute. FYI: Yet another way of dealing with this is to set up _R_CHECK_EXAMPLE_TIMING_THRESHOLD_ env var

m7pr · 2023-06-29T08:10:58Z

@pawelru @gogonzo what if we setup R_CHECK_EXAMPLE_TIMING_THRESHOLD to 10 seconds? And for the decoupling, we can have more time to rethink - I would suggest storing minimal SCDA data in teal/data directory and reuse it in all teal.* family packages that use teal. This way we keep good examples, based on clinical/scda data and we got rid of scda

pawelru · 2023-06-29T08:14:29Z

@m7pr I am not that deep in this package so can't say how hard is it to do the optimisation. Please make a call. I trust your assessment

m7pr · 2023-06-29T08:20:17Z

@gogonzo what would you say for bumping up the threshold time for examples in here to 10 seconds?
https://github.com/insightsengineering/r.pkg.template/blob/main/.github/workflows/build-check-install.yaml#L410

And for the decoupling I think we could have a separate discussion as I rather recommend to have scda data stored in teal and reused in mutltiple packages, instead of having examples on iris

chlebowa · 2023-06-29T08:23:06Z

@gogonzo what would you say for bumping up the threshold time for examples in here to 10 seconds? https://github.com/insightsengineering/r.pkg.template/blob/main/.github/workflows/build-check-install.yaml#L410

I think this is a good temporary solution, the threshold can be lowered once #532 is completed.

And for the decoupling I think we could have a separate discussion as I rather recommend to have scda data stored in teal and reused in mutltiple packages, instead of having examples on iris

A similar solution was suggested and explicitly rejected.

m7pr · 2023-06-29T08:28:56Z

yeah, I see that, we would need to use scda::cdisc_data anyway to pass to teal::init if we ever stored scda in teal/data directory. So I guess we go with mtcars/iris.

I made a draft to increase examples time - waiting for @gogonzo for a green light insightsengineering/r.pkg.template#167

pawelru · 2023-06-29T08:32:30Z

I made a draft to increase examples time - waiting for @gogonzo for a green light insightsengineering/r.pkg.template#167

Please do this for this repo only. I would like to stick to defaults for all the other repos and treat this one as an exception (out of that default value of 5)

m7pr · 2023-06-29T08:52:13Z

ok @pawelru , I made a PR to this repository - maybe you can review and accept #533

gogonzo · 2023-06-29T10:19:24Z

I wouldn't keep data in teal as we have following dependency order:

teal.widgets
teal.code
teal.data (use scda)
teal.transform (use scda)
teal.slice (use scda)
teal (use scda and all packages above)
teal.modules.xyz (use teal and teal.transform)

The fact that teal.transform uses scda is unfortunate and it will be removed from there soon. In fact, only packages which need artifical clinical data are teal.modules.clinical, osprey, teal.osprey, goshawk, teal.goshawk and perhaps teal.modules.hermes.

Optimally, data could be stored in teal.data (as name of the package suggests) but teal.data in current form is not a dependency of the teal.modules.xyz. But in a long term I expect teal.data to have ONE reproducible data class which could be used in whole teal application from setting teal::init(data, ...) to teal.module.xyz::tm_module_srv(data, ...) - It would make teal.data being a dependency of all teal.xyz packages.

#521 teal.code version bump

cf5e2c3

m7pr added documentation Improvements or additions to documentation core labels Jun 26, 2023

m7pr requested a review from gogonzo June 26, 2023 13:59

m7pr marked this pull request as ready for review June 26, 2023 13:59

m7pr and others added 2 commits June 27, 2023 11:57

#521 fix separator for missing packages in checks for tm_g_distributi…

5387684

…on and tm_g_scatterplot

Merge 5387684 into 9f9067c

0b5acdd

[skip actions] Restyle files

4e114e5

m7pr added 3 commits June 27, 2023 13:47

fix #211

49d0c1b

Merge branch '521_review_documentation@main' of https://github.com/in…

99732e8

…sightsengineering/teal.modules.general into 521_review_documentation@main

revert - changes made to the wrong package

cfc14f4

gogonzo approved these changes Jun 28, 2023

View reviewed changes

Merge branch 'main' into 521_review_documentation@main

48fc620

Merge branch 'main' into 521_review_documentation@main

ec8b4d6

m7pr merged commit ed981c4 into main Jun 29, 2023

m7pr deleted the 521_review_documentation@main branch June 29, 2023 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

521 documentation review before the release #530

521 documentation review before the release #530

m7pr commented Jun 26, 2023 •

edited

Loading

github-actions bot commented Jun 26, 2023 •

edited

Loading

github-actions bot commented Jun 26, 2023 •

edited

Loading

gogonzo commented Jun 27, 2023

m7pr commented Jun 27, 2023

chlebowa commented Jun 27, 2023

m7pr commented Jun 27, 2023 •

edited

Loading

gogonzo commented Jun 27, 2023

chlebowa commented Jun 27, 2023

m7pr commented Jun 27, 2023

m7pr commented Jun 27, 2023

chlebowa commented Jun 27, 2023

m7pr commented Jun 27, 2023 •

edited

Loading

chlebowa commented Jun 27, 2023

m7pr commented Jun 27, 2023

gogonzo left a comment

m7pr commented Jun 28, 2023 •

edited

Loading

gogonzo commented Jun 28, 2023

m7pr commented Jun 29, 2023

pawelru commented Jun 29, 2023

m7pr commented Jun 29, 2023

pawelru commented Jun 29, 2023

m7pr commented Jun 29, 2023

chlebowa commented Jun 29, 2023

m7pr commented Jun 29, 2023

pawelru commented Jun 29, 2023

m7pr commented Jun 29, 2023

gogonzo commented Jun 29, 2023 •

edited

Loading

521 documentation review before the release #530

521 documentation review before the release #530

Conversation

m7pr commented Jun 26, 2023 • edited Loading

github-actions bot commented Jun 26, 2023 • edited Loading

Code Coverage Summary

Diff against main

github-actions bot commented Jun 26, 2023 • edited Loading

Unit Tests Summary

gogonzo commented Jun 27, 2023

m7pr commented Jun 27, 2023

chlebowa commented Jun 27, 2023

m7pr commented Jun 27, 2023 • edited Loading

gogonzo commented Jun 27, 2023

chlebowa commented Jun 27, 2023

m7pr commented Jun 27, 2023

m7pr commented Jun 27, 2023

chlebowa commented Jun 27, 2023

m7pr commented Jun 27, 2023 • edited Loading

chlebowa commented Jun 27, 2023

m7pr commented Jun 27, 2023

gogonzo left a comment

Choose a reason for hiding this comment

m7pr commented Jun 28, 2023 • edited Loading

gogonzo commented Jun 28, 2023

m7pr commented Jun 29, 2023

pawelru commented Jun 29, 2023

m7pr commented Jun 29, 2023

pawelru commented Jun 29, 2023

m7pr commented Jun 29, 2023

chlebowa commented Jun 29, 2023

m7pr commented Jun 29, 2023

pawelru commented Jun 29, 2023

m7pr commented Jun 29, 2023

gogonzo commented Jun 29, 2023 • edited Loading

m7pr commented Jun 26, 2023 •

edited

Loading

github-actions bot commented Jun 26, 2023 •

edited

Loading

github-actions bot commented Jun 26, 2023 •

edited

Loading

m7pr commented Jun 27, 2023 •

edited

Loading

m7pr commented Jun 27, 2023 •

edited

Loading

m7pr commented Jun 28, 2023 •

edited

Loading

gogonzo commented Jun 29, 2023 •

edited

Loading