Skip to content

Conversation

@gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Jan 22, 2026

Assertion in c.teal_slices is too strict as it doesn't recognise that two teal_slice objects have identical values.
In following example, we throw an error (unnecessary) as both teal_slice object have identical values.

library(teal)
c(
  teal.slice::teal_slices(
    teal.slice::teal_slice("ADSL", varname = "AGE", id = "ADSL_AGE")
  ),
  teal.slice::teal_slices(
    teal.slice::teal_slice("ADSL", varname = "AGE", id = "ADSL_AGE")
  )
)

However, situation when somebody specifies duplicated-by-id slices, having different values, then we should throw an error

library(teal)
c(
  teal.slice::teal_slices(
    teal.slice::teal_slice("ADSL", varname = "AGE", id = "ADSL_AGE")
  ),
  teal.slice::teal_slices(
    teal.slice::teal_slice("ADSL", varname = "SEX", id = "ADSL_AGE")
  )
)

@gogonzo gogonzo added the core label Jan 22, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2026

Unit Tests Summary

  1 files   31 suites   43s ⏱️
411 tests 396 ✅ 15 💤 0 ❌
872 runs  857 ✅ 15 💤 0 ❌

Results for commit d9a468f.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2026

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
teal_slices 👶 $+0.01$ teal_slices_accepts_duplicated_elements_if_they_have_identical_values
teal_slices 👶 $+0.01$ teal_slices_throw_an_error_if_elements_with_duplicated_id_have_different_settings

Results for commit 304fce9

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2026

badge

Code Coverage Summary

Filename                        Stmts    Miss  Cover    Missing
----------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------
R/calls_combine_by.R                7       0  100.00%
R/choices_labeled.R                23       1  95.65%   34
R/count_labels.R                  126       0  100.00%
R/filter_panel_api.R               27       1  96.30%   132
R/FilteredData-utils.R             58      17  70.69%   38-43, 139, 161-170
R/FilteredData.R                  522     118  77.39%   109, 138, 186, 325, 397, 484-493, 514, 532-590, 610-613, 657, 712-728, 873, 885-905
R/FilteredDataset-utils.R          23       1  95.65%   125
R/FilteredDataset.R               264       5  98.11%   49, 154, 189, 214-215
R/FilteredDatasetDataframe.R      123       3  97.56%   87, 148, 158
R/FilteredDatasetDefault.R         18       4  77.78%   104-117
R/FilteredDatasetMAE.R            133      15  88.72%   56, 118-123, 160-165, 169-170
R/FilterPanelAPI.R                 10       0  100.00%
R/FilterState-utils.R             101       2  98.02%   264, 294
R/FilterState.R                   366      21  94.26%   91, 155, 266-267, 273-274, 328, 330, 416, 632, 675-679, 698, 762-768, 779
R/FilterStateChoices.R            349      10  97.13%   303, 426-429, 541-544, 589
R/FilterStateDate.R               221      46  79.19%   233, 361-371, 382-387, 391-396, 404-419, 438-445
R/FilterStateDatettime.R          316     206  34.81%   269, 321-559
R/FilterStateEmpty.R               53      31  41.51%   89, 99-104, 117, 129-169
R/FilterStateExpr.R                81       1  98.77%   244
R/FilterStateLogical.R            201     149  25.87%   139, 161, 221, 225-414
R/FilterStateRange.R              409     121  70.42%   265, 387, 498-505, 508-518, 521, 532-538, 549-561, 565-575, 579-581, 594-620, 635-642, 645-652, 666-683, 718-723, 733-735
R/FilterStates-utils.R             70       7  90.00%   108, 127, 188-194
R/FilterStates.R                  379      19  94.99%   63, 92-96, 208, 425, 470, 559-563, 608, 726-729
R/FilterStatesDF.R                  5       0  100.00%
R/FilterStatesMAE.R                10       1  90.00%   40
R/FilterStatesMatrix.R              7       0  100.00%
R/FilterStatesSE.R                171      47  72.51%   36, 73-75, 85-87, 119, 191-201, 213-223, 230-237, 245-252, 259
R/include_css_js.R                  5       0  100.00%
R/teal_slice.R                    108       1  99.07%   206
R/teal_slices.R                    88       5  94.32%   152-157
R/test_utils.R                     21       0  100.00%
R/utils.R                          29       0  100.00%
R/variable_types.R                 15       1  93.33%   48
R/zzz.R                            16      16  0.00%    3-46
TOTAL                            4355     849  80.51%

Diff against main

Filename           Stmts    Miss  Cover
---------------  -------  ------  -------
R/teal_slices.R       +3       0  +0.20%
TOTAL                 +3       0  +0.01%

Results for commit: d9a468f

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Looks good simple and to the point

@llrs-roche llrs-roche self-assigned this Jan 22, 2026
@gogonzo
Copy link
Contributor Author

gogonzo commented Jan 23, 2026

@llrs-roche thanks for the review. Decided to move duplicates-removal from c to teal_slice (c calls teal_slice). Thanks to this we control duplicates in one place and both c() and teal_slices ignore duplicates

@gogonzo gogonzo merged commit 17d358d into main Jan 23, 2026
29 checks passed
@gogonzo gogonzo deleted the 1683_ignore_duplicated_slice branch January 23, 2026 13:22
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request]: c of teal.slices should treat duplicated teal_slice as a shared filters

3 participants