Decoupled scda #534

kartikeyakirar · 2023-07-07T13:36:38Z

fixes #532

Related PRs
insightsengineering/scda#128
insightsengineering/scda.2022#127

github-actions · 2023-07-07T13:48:40Z

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  ----------------------------------------------
R/tm_a_pca.R                    821     821  0.00%    72-1018
R/tm_a_regression.R             710     710  0.00%    92-886
R/tm_data_table.R               171     171  0.00%    58-281
R/tm_file_viewer.R              172     172  0.00%    39-241
R/tm_front_page.R               127     116  8.66%    63-205
R/tm_g_association.R            327     327  0.00%    89-475
R/tm_g_bivariate.R              652     492  24.54%   124-702, 750, 756, 760, 871, 888, 906, 926-948
R/tm_g_distribution.R          1025    1025  0.00%    110-1257
R/tm_g_response.R               346     346  0.00%    85-495
R/tm_g_scatterplot.R            716     716  0.00%    142-949
R/tm_g_scatterplotmatrix.R      278     259  6.83%    78-373, 427, 441
R/tm_missing_data.R            1034    1034  0.00%    58-1228
R/tm_outliers.R                 944     944  0.00%    76-1142
R/tm_t_crosstable.R             251     251  0.00%    81-370
R/tm_variable_browser.R         815     810  0.61%    61-1293
R/utils.R                       122     122  0.00%    74-351
R/zzz.R                           1       1  0.00%    2
TOTAL                          8512    8317  2.29%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 137fcc7

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2023-07-11T08:10:59Z

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  ----------------------------------------------
R/tm_a_pca.R                    828     828  0.00%    72-1024
R/tm_a_regression.R             712     712  0.00%    92-887
R/tm_data_table.R               171     171  0.00%    58-281
R/tm_file_viewer.R              172     172  0.00%    39-241
R/tm_front_page.R               127     116  8.66%    63-205
R/tm_g_association.R            331     331  0.00%    89-478
R/tm_g_bivariate.R              652     492  24.54%   124-702, 750, 756, 760, 871, 888, 906, 926-948
R/tm_g_distribution.R          1025    1025  0.00%    110-1257
R/tm_g_response.R               347     347  0.00%    85-496
R/tm_g_scatterplot.R            716     716  0.00%    160-967
R/tm_g_scatterplotmatrix.R      278     259  6.83%    78-373, 427, 441
R/tm_missing_data.R            1034    1034  0.00%    58-1228
R/tm_outliers.R                 944     944  0.00%    76-1142
R/tm_t_crosstable.R             252     252  0.00%    81-371
R/tm_variable_browser.R         815     810  0.61%    61-1293
R/utils.R                       122     122  0.00%    74-351
R/zzz.R                           1       1  0.00%    2
TOTAL                          8527    8332  2.29%

Diff against main

Filename                Stmts    Miss  Cover
--------------------  -------  ------  --------
R/tm_a_pca.R               +7      +7  +100.00%
R/tm_a_regression.R        +2      +2  +100.00%
R/tm_g_association.R       +4      +4  +100.00%
R/tm_g_response.R          +1      +1  +100.00%
R/tm_t_crosstable.R        +1      +1  +100.00%
TOTAL                     +15     +15  -0.00%

Results for commit: b071192

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2023-07-11T08:11:49Z

Unit Tests Summary

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

Results for commit 2aeceb7.

♻️ This comment has been updated with latest results.

m7pr

Hey @kartikeyakirar I left a few general comments that I would like you to apply in multiple places. This is good enough, but requires small changes.
Also, can you put the code that you used to create datasets in dev/data-creation.R or in data-raw/data-creation.R file? And add this file to .Rbuildignore? See this section . This way we can track properties of data and we can recreate data in the future if they need slight changes

R/data.R

R/tm_a_pca.R

R/tm_a_regression.R

m7pr · 2023-07-11T08:44:43Z

@kartikeyakirar one more thing. Look at this PR where we decoupled scda from teal. We also decoupled teal from scda in staged.dependencies yaml. Would you do the same reverse PRs in scda and scda.2022 as in the linked PR did for teal?

kartikeyakirar · 2023-07-11T08:54:42Z

@m7pr

Also, can you put the code that you used to create datasets in dev/data-creation.R or in data-raw/data-creation.R file? And add this file to .Rbuildignore? See this section . This way we can track properties of data and we can recreate data in the future if they need slight changes

I think the process of data creation will reintegrate SCDA dependencies, as data is stored from the SCDA package. This is consistent across all packages. However, it is worth considering the possibility of consolidating all the data into a single package. While the current approach is in use, it may be beneficial to have a discussion about centralizing the data within a specific package.

m7pr · 2023-07-11T09:11:28Z

@kartikeyakirar this is fine. Just use the code and then comment it out complitely so we can track how the data was created. Can you also document the format? What are the column names and what they represent? Like here https://github.com/tidyverse/nycflights13/blob/main/R/weather.R#L7

kartikeyakirar · 2023-07-11T09:21:27Z

I initially thought to format them but there are more or less 55 cols in ADSL itself. I don't have descriptions for all of them as well.

m7pr · 2023-07-11T09:22:49Z

Ok, so let's forget about the format section

…

On Tue, Jul 11, 2023 at 11:21 AM kartikeya kirar ***@***.***> wrote: I initially thought to format them but there are more or less 55 cols. I don't have description for all of them as well. — Reply to this email directly, view it on GitHub <#534 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A74AIEPGHCIQGHJLBQUAYLLXPULKFANCNFSM6AAAAAA2B2TEVM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

kartikeyakirar · 2023-07-11T09:23:31Z

@kartikeyakirar one more thing. insightsengineering/teal#858 (comment) where we decoupled scda from teal. We also decoupled teal from scda in staged.dependencies yaml. Would you do the same reverse PRs in scda and scda.2022 as in the linked PR did for teal?

Yes , for this I will raise PR along with teal.data to remove downstream dependencies.

m7pr · 2023-07-11T09:29:23Z

Great, since the merge here needs to happen at the same time as for scda/scda.2022

…eering/teal.modules.general into decouple_scda@main

m7pr

requested changes were not solved

…eation

m7pr

@kartikeyakirar a couple more minor comments.

1

Mostly, are you able to append init functions with proper package prefix teal? The same for modules/module function? In all examples? The same goes for actual usage of tm_g_* and tm_a_* functions. If they are used, are you able to append package name in front of them (teal.modules.general)?

2

One more though, are you able to rewrite examples, so that we only use rADSL and rADLB? This way we have less datasets created and we can try removing that on other packages, so that we use only those two. This is just a loos thought and I'd like your honest opinion on that.

R/tm_a_regression.R

R/tm_data_table.R

R/tm_front_page.R

R/tm_g_bivariate.R

vignettes/using-cross-table.Rmd

m7pr · 2023-07-12T09:46:47Z

@gogonzo @kartikeyakirar do we need to create datasets inside this package (teal.modules.general) if we actually can use the same datasets created for teal.transform? So instead of creating datasets here and calling them with teal.modules.general::rADAE, we can get rid of data here and just use teal.transform::rADAE ?

gogonzo · 2023-07-12T11:05:15Z

@m7pr @kartikeyakirar let's not make links between packages because of data. Ideally in the future most of the packages should be independent from each other and data-dependency would be another thing to decouple.

kartikeyakirar · 2023-07-12T11:35:03Z

One more though, are you able to rewrite examples, so that we only use rADSL and rADLB? This way we have less datasets created and we can try removing that on other packages, so that we use only those two. This is just a loos thought and I'd like your honest opinion on that.

The examples were created using these datasets, and choices and selections were made accordingly. It is essential to thoroughly examine all these datasets to understand how they are used in the examples. Additionally, the RDA files' overall size was quite small, so I chose not to refactor them.

What is your thought @gogonzo? We also have eight datasets in teal.data.

…eering/teal.modules.general into decouple_scda@main

R/tm_missing_data.R

adding namesapce call :: for teal.modules.general and teal.tranform

…eering/teal.modules.general into decouple_scda@main

m7pr · 2023-07-12T14:32:28Z

@kartikeyakirar let's leave those 5 datasets for now

m7pr

great job. team work makes the dream work

A follow-up after #139 that closed #133 After I had a chance to do a scda decoupling for goshawk insightsengineering/goshawk#198 and had a chance to review scda decoupling for teal.modules.general insightsengineering/teal.modules.general#534 I realized some changes need to be applied in PRs that were already merged in other packages. Main changes: - I prepend dataset names with package names as now we are having the same data in multiple packages (`teal.transform::rADAE` and `teal.modules.general::rADAE` for example) - added a `data-raw/data.R` file to show how the `data/` folder was created - extended `.RBuildignore` file to omit `data-raw/data.R` while building the package

kartikeya and others added 2 commits July 7, 2023 19:05

Decoupled scda pkg calls

5d534b4

Merge 5d534b4 into 0d54f19

137fcc7

kartikeyakirar added the core label Jul 7, 2023

[skip actions] Restyle files

55f069d

m7pr self-requested a review July 11, 2023 08:03

Empty-Commit

74febe8

m7pr requested changes Jul 11, 2023

View reviewed changes

R/data.R Outdated Show resolved Hide resolved

R/tm_a_pca.R Outdated Show resolved Hide resolved

R/tm_a_pca.R Outdated Show resolved Hide resolved

R/tm_a_pca.R Show resolved Hide resolved

R/tm_a_regression.R Outdated Show resolved Hide resolved

kartikeya and others added 7 commits July 11, 2023 15:25

adding library call

8317d53

Merge branch 'decouple_scda@main' of https://github.com/insightsengin…

6a830f8

…eering/teal.modules.general into decouple_scda@main

fxing indentation

6da5172

Merge 6da5172 into 0d54f19

a257349

[skip actions] Restyle files

90143e7

fixing indentation issue

40d9d7e

Merge 40d9d7e into 0d54f19

082962a

kartikeyakirar requested a review from m7pr July 11, 2023 11:38

github-actions bot and others added 3 commits July 11, 2023 11:41

[skip actions] Restyle files

7fac1ea

styling codition statment

95ee0fc

Merge 95ee0fc into 0d54f19

011e99a

kartikeyakirar marked this pull request as draft July 11, 2023 13:59

[skip actions] Restyle files

3f98314

m7pr requested changes Jul 11, 2023

View reviewed changes

kartikeya and others added 2 commits July 11, 2023 20:40

updating package call for internal data. and added script for data cr…

29a3d9a

…eation

Merge 29a3d9a into 0d54f19

9866227

kartikeyakirar marked this pull request as ready for review July 11, 2023 15:12

[skip actions] Restyle files

b057d19

kartikeyakirar requested a review from m7pr July 11, 2023 15:21

m7pr requested changes Jul 12, 2023

View reviewed changes

R/tm_a_regression.R Outdated Show resolved Hide resolved

R/tm_data_table.R Outdated Show resolved Hide resolved

R/tm_front_page.R Outdated Show resolved Hide resolved

R/tm_g_bivariate.R Outdated Show resolved Hide resolved

vignettes/using-cross-table.Rmd Outdated Show resolved Hide resolved

m7pr added 2 commits July 12, 2023 11:13

rewrite nolint in vignettes

a0d16f3

append #nolint for the long if for greater readability

fa7c19d

m7pr mentioned this pull request Jul 12, 2023

decouple scda followup insightsengineering/teal.transform#142

Merged

kartikeya added 2 commits July 12, 2023 17:22

adding namesapce call ::

9658646

Merge branch 'decouple_scda@main' of https://github.com/insightsengin…

348b0ce

…eering/teal.modules.general into decouple_scda@main

m7pr reviewed Jul 12, 2023

View reviewed changes

R/tm_missing_data.R Outdated Show resolved Hide resolved

m7pr reviewed Jul 12, 2023

View reviewed changes

R/tm_missing_data.R Outdated Show resolved Hide resolved

kartikeya and others added 4 commits July 12, 2023 17:59

adding nolint for vignettes code section and

31dfc78

adding namesapce call :: for teal.modules.general and teal.tranform

Merge 31dfc78 into 0d54f19

987e173

[skip actions] Restyle files

5c5583b

'Empty-Commit'

233bbd5

kartikeyakirar requested a review from m7pr July 12, 2023 12:58

dependabot-preview bot and others added 3 commits July 12, 2023 13:02

[skip actions] Roxygen Man Pages Auto Update

b3c19b4

'Empty-Commit'

5ade0ad

Merge branch 'decouple_scda@main' of https://github.com/insightsengin…

2aeceb7

…eering/teal.modules.general into decouple_scda@main

m7pr approved these changes Jul 12, 2023

View reviewed changes

kartikeyakirar merged commit 194c9d8 into main Jul 12, 2023
24 checks passed

kartikeyakirar deleted the decouple_scda@main branch July 12, 2023 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decoupled scda #534

Decoupled scda #534

kartikeyakirar commented Jul 7, 2023 •

edited

Loading

github-actions bot commented Jul 7, 2023

github-actions bot commented Jul 11, 2023 •

edited

Loading

github-actions bot commented Jul 11, 2023 •

edited

Loading

m7pr left a comment

m7pr commented Jul 11, 2023

kartikeyakirar commented Jul 11, 2023 •

edited

Loading

m7pr commented Jul 11, 2023

kartikeyakirar commented Jul 11, 2023 •

edited

Loading

m7pr commented Jul 11, 2023 via email

kartikeyakirar commented Jul 11, 2023

m7pr commented Jul 11, 2023

m7pr left a comment

m7pr left a comment

m7pr commented Jul 12, 2023

gogonzo commented Jul 12, 2023

kartikeyakirar commented Jul 12, 2023

m7pr commented Jul 12, 2023

m7pr left a comment

Decoupled scda #534

Decoupled scda #534

Conversation

kartikeyakirar commented Jul 7, 2023 • edited Loading

github-actions bot commented Jul 7, 2023

Code Coverage Summary

Diff against main

github-actions bot commented Jul 11, 2023 • edited Loading

Code Coverage Summary

Diff against main

github-actions bot commented Jul 11, 2023 • edited Loading

Unit Tests Summary

m7pr left a comment

Choose a reason for hiding this comment

m7pr commented Jul 11, 2023

kartikeyakirar commented Jul 11, 2023 • edited Loading

m7pr commented Jul 11, 2023

kartikeyakirar commented Jul 11, 2023 • edited Loading

m7pr commented Jul 11, 2023 via email

kartikeyakirar commented Jul 11, 2023

m7pr commented Jul 11, 2023

m7pr left a comment

Choose a reason for hiding this comment

m7pr left a comment

Choose a reason for hiding this comment

1

2

m7pr commented Jul 12, 2023

gogonzo commented Jul 12, 2023

kartikeyakirar commented Jul 12, 2023

m7pr commented Jul 12, 2023

m7pr left a comment

Choose a reason for hiding this comment

kartikeyakirar commented Jul 7, 2023 •

edited

Loading

github-actions bot commented Jul 11, 2023 •

edited

Loading

github-actions bot commented Jul 11, 2023 •

edited

Loading

kartikeyakirar commented Jul 11, 2023 •

edited

Loading

kartikeyakirar commented Jul 11, 2023 •

edited

Loading