FilteredData constructor #49

nikolas-burkoff · 2022-06-14T11:25:56Z

Closes #33

kpagacz · 2022-06-14T13:50:51Z

I do not see a reason to abandon dispatching on TealData or whatever class inherits from it. I think the smart thing to do is to add an ability to dispatch on things other than TealData and leave the current dispatch intact. This conviction stems from my opinion on what teal should do to data passed to the data parameter of teal::init, which is ideally nothing. Leave the data as it is, let the filter panel dispatch on this thing, and it can decide what it can do. In extreme cases, I can imagine a greyed-out filter panel because whatever user passed to data is not supported by the panel, so it basically exposes nothing.

If we do not let it dispatch, then teal has to worry about what to pass to the filter panel to make it work, and it should not be the teal's decision to make. The panel knows best what it can and cannot work with. Otherwise, we get otherworldly calisthenics in teal or teal.data to make it work, like here: https://github.com/insightsengineering/teal/blob/865c01a64b13de7e4d0c6f333efd37e5604fd5f9/R/module_teal.R#L189 (from PR https://github.com/insightsengineering/teal/pull/670/files)

This also would (fingers crossed?) let users create their own init_filtered_state dispatch that could work as an interface between their weird data and the filter panel.

R/CDISCFilteredData.R

gogonzo

Filtering parent doesn't affect the child. See efficacy app for example. get_call in CDISCFilteredData has been modified but this call isn't executed anywhere. This means that relationship between reactive parent and child has been lost.

nikolas-burkoff · 2022-06-16T16:07:39Z

tests/testthat/test-FilteredData.R

  testthat::expect_error(isolate(datasets$get_filter_overview("AA")), "Some datasets are not available:")
  testthat::expect_error(isolate(datasets$get_filter_overview("")), "Some datasets are not available:")
  testthat::expect_error(isolate(datasets$get_filter_overview(23)), "Some datasets are not available:")
 })

 testthat::test_that("get_filter_overview returns overview matrix for non-filtered datasets", {
+  datasets <- get_filtered_data_object()


So this test and the following tests fail.

This is what is expected:

But we can no longer distinguish that mock_iris is not CDISC and ADSL is CDISC with no parent. Previously the former was DefaultFilteredDataset and the latter CDISCFilteredDataset. Now they are both DefaultFilteredDataset and then either they both have subjs as this branch now creates a CDISCFilteredData object:

Or if I change the creation of datasets to use FilteredData we get neither with subjs:

Is this change of behaviour OK? More generally it seems like the design we've changed to is fighting against the dispatch mechanisms in R (we have cdisc, non cdisc and MAE data, on the dataset level we have MAE and not MAE and on the data level we have CDISC and not CDISC). I'm OK with this if it's a stepping stone to simplifying the filter panel and removing all the data but I don't really like it Especially trying to understand the logic to get the filter information - maybe this logic can be simplified?

R/FilteredData.R

gogonzo · 2022-06-17T06:36:45Z

R/FilteredData.R

+    #' TODO
+    #' @param keys TODO
+    #' @return (`self`) invisibly this `FilteredData`
+    set_join_keys = function(keys) {


this should be a private now

github-actions · 2022-06-20T15:26:43Z

Unit Tests Summary

    1 files   28 suites 15s ⏱️
414 tests 414 ✔️ 0 💤 0 ❌
719 runs 719 ✔️ 0 💤 0 ❌

Results for commit 649fb6d.

♻️ This comment has been updated with latest results.

github-actions · 2022-06-20T15:26:58Z

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/call_utils.R                  156       4  97.44%   142-145
R/CDISCFilteredData.R           153      27  82.35%   113, 122, 126, 140-149, 251-258, 276-279, 285, 350-353
R/choices_labeled.R              49      14  71.43%   19, 30, 35, 45-50, 62, 66-70
R/eval_expr_with_msg.R           16       6  62.50%   8-13
R/filtered_data_wrappers.R       17      17  0.00%    134-169
R/FilteredData.R                440     310  29.55%   131, 215-219, 253, 255, 257, 260, 275-289, 330-344, 464-466, 593-975, 999, 1006-1007
R/FilteredDataset.R             250      84  66.40%   100, 187, 283, 333-373, 442-448, 489-499, 664-706
R/FilterState.R                1362     718  47.28%   160, 353-354, 451, 636-650, 712-762, 903-930, 1068-1177, 1202-1208, 1382-1542, 1666-1671, 1675-1681, 1694, 1698, 1830-1947, 2004-2010, 2023, 2155-2272, 2300-2306, 2319, 2405-2407, 2473-2634, 2663-2669, 2683
R/FilterStates.R               1413     595  57.89%   101, 119, 223, 302, 381, 432-618, 632-633, 751-787, 942-951, 1008, 1074, 1115, 1176-1181, 1316-1437, 1452, 1486, 1554-1559, 1579-1585, 1592-1597, 1604-1605, 1607, 1768-1777, 1810-1817, 1825-1832, 1860-2029, 2069-2075, 2092-2098, 2105-2110, 2118-2119, 2121, 2169-2170, 2251-2369, 2416, 2449, 2470
R/include_css_js.R                5       5  0.00%    12-16
R/init_filitered_data.R          62       0  100.00%
R/MAEFilteredDataset.R          212      59  72.17%   25, 122, 254-330
R/Queue.R                        50      23  54.00%   22, 40-72, 96-113, 154
R/resolve_state.R                23       0  100.00%
R/utils.R                        74      13  82.43%   123-129, 141-146
R/variable_types.R               48      33  31.25%   42-47, 57, 70-105
R/zzz.R                           6       6  0.00%    3-11
TOTAL                          4336    1914  55.86%

Results for commit: 783c15f

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

R/FilteredData.R

nikolas-burkoff · 2022-06-21T10:05:30Z

@gogonzo - your review is probably stale now :)

add issue to update filter-panel.Rmd currently I've just added a note saying what is out of date

I guess we don't need to do this? As everything is going to be changing a lot now?

work out a nice way of specifying keys/join keys for cdisc data so you don't have to specify them all - although this could go to a different issue?

Do you think we need to do this? We could have join_keys = teal::get_cdisc_join_keys() to simplify creating a CDISCFilteredData object - if so I'll create an issue

kpagacz · 2022-06-21T11:37:39Z

What happens with MAE right now? Or are we doing it in another PR?

EDIT: I think there might be a problem with mixing data.frame and MAE objects together with some join keys between them or parent/child relationship. Pretty sure it is going to go wrong.

nikolas-burkoff · 2022-06-21T11:46:59Z

EDIT: I think there might be a problem with mixing data.frame and MAE objects together with some join keys between them or parent/child relationship. Pretty sure it is going to go wrong.

So I think it is going to go the same wrong as it would have done before this PR? Maybe an issue in the backlog?

kpagacz · 2022-06-21T12:59:41Z

R/FilteredData.R

@@ -104,7 +134,7 @@ FilteredData <- R6::R6Class( # nolint
    #' @description
    #' Gets variable names of a given dataname for the filtering.
    #'
-    #' @param dataname (`character`) name of the dataset
+    #' @param dataname (`character(1)`) name of the dataset
    #' @return (`character` vector) of variable names
    get_filterable_varnames = function(dataname) {


Btw, what is the point of this method? XD

it's overwritten in CDISCFilteredData...

in FilteredData it doesn't do anything. colnames(data) == filterable_varnames

In CDISCFIltereData we don't want child to have duplicated columns. It's actually users request because in ADTTE there are some duplicated columns comparing to ADSL. I foreseen that in the future it might cause some confusion as we can for example build a relationship between movie <-> actor and movie.name with actor.name are duplicated names but not the same thing. Let's discuss this in some time in the future if it will be important.

…sengineering/teal.slice into 33_filteredData_constructor@main

gogonzo · 2022-06-21T14:45:53Z

add issue to update filter-panel.Rmd currently I've just added a note saying what is out of date
I guess we don't need to do this? As everything is going to be changing a lot now?

We need the update for sure. I'll put this to sprint 60 when we have everything possibly clarified and done.

#52

work out a nice way of specifying keys/join keys for cdisc data so you don't have to specify them all - although this could go to a different issue?
Do you think we need to do this? We could have join_keys = teal::get_cdisc_join_keys() to simplify creating a CDISCFilteredData object - if so I'll create an issue

Let's create the issue but doesn't have to be taken in this increment. #53

gogonzo · 2022-06-21T14:48:28Z

EDIT: I think there might be a problem with mixing data.frame and MAE objects together with some join keys between them or parent/child relationship. Pretty sure it is going to go wrong.

So I think it is going to go the same wrong as it would have done before this PR? Maybe an issue in the backlog?

@nikolas-burkoff is right. Nothing new. If the MAE users complain about this we would know. At this moment it's a gap we can ignore.

out of date review

…/insightsengineering/teal.slice into 33_filteredData_constructor@main

gogonzo

Setting FilteredData is simplified as much as it's possible at this moment.

kpagacz

I have spent the morning launching the apps from the gallery and I have not noticed anything browfurrowing.

tests/testthat/test-MAEFilteredDataset.R

Nikolas Burkoff added 3 commits June 10, 2022 14:16

update FilteredDataset

f472009

filtereddata

773c16c

vignettes and docs

49636eb

nikolas-burkoff marked this pull request as draft June 14, 2022 11:26

nikolas-burkoff added the core label Jun 14, 2022

gogonzo self-requested a review June 14, 2022 12:23

gogonzo self-assigned this Jun 14, 2022

nikolas-burkoff mentioned this pull request Jun 14, 2022

changes for filter panel constuctor insightsengineering/teal#670

Merged

udpate docs

aaf3597

udpates

347e446

This comment was marked as outdated.

Sign in to view

Nikolas Burkoff added 4 commits June 14, 2022 16:31

update docs

b14340e

add TODO

cd11afc

fix

36edeea

to fix teal.transform

2ec6968

nikolas-burkoff mentioned this pull request Jun 15, 2022

updates for teal.slice insightsengineering/teal.transform#73

Merged

gogonzo reviewed Jun 15, 2022

View reviewed changes

R/CDISCFilteredData.R Show resolved Hide resolved

Nikolas Burkoff added 2 commits June 15, 2022 15:12

fix some tests

ce3cee9

fix more tests

27db29a

gogonzo previously requested changes Jun 15, 2022

View reviewed changes

Nikolas Burkoff and others added 5 commits June 15, 2022 16:42

tests

a90c8c0

Fix@33 filtered data constructor@main (#51)

1ede5ee

Merge branch 'main' into 33_filteredData_constructor@main

cc8b2d8

more tests

4c7a98d

update for MAE inside CDISCFilteredData

58b75ab

nikolas-burkoff commented Jun 16, 2022

View reviewed changes

Dawid comment

7d98a0a

gogonzo reviewed Jun 17, 2022

View reviewed changes

R/FilteredData.R Show resolved Hide resolved

gogonzo reviewed Jun 17, 2022

View reviewed changes

kpagacz self-assigned this Jun 21, 2022

docs, validation, spelling

2f5680b

kpagacz reviewed Jun 21, 2022

View reviewed changes

R/FilteredData.R Outdated Show resolved Hide resolved

default args

2aedda7

kpagacz reviewed Jun 21, 2022

View reviewed changes

kpagacz added 2 commits June 21, 2022 15:23

some test gaps filled and TODO done

5a533e8

Merge branch '33_filteredData_constructor@main' of github.com:insight…

bfb6de8

…sengineering/teal.slice into 33_filteredData_constructor@main

Nikolas Burkoff added 3 commits June 21, 2022 16:01

pkgdown

21779e7

Merge branch '33_filteredData_constructor@main' of https://github.com…

e796a4d

…/insightsengineering/teal.slice into 33_filteredData_constructor@main

linter

7fe43e9

gogonzo approved these changes Jun 21, 2022

View reviewed changes

move MAEFilteredDataset into its own file

dcaf348

kpagacz approved these changes Jun 22, 2022

View reviewed changes

Nikolas Burkoff and others added 3 commits June 22, 2022 08:53

fix varlabels for MAE dataset

81eb2eb

update tests

e4d3d9d

added more tests for init_filtered_data.default

767eaaf

kpagacz reviewed Jun 22, 2022

View reviewed changes

tests/testthat/test-MAEFilteredDataset.R Outdated Show resolved Hide resolved

kpagacz and others added 3 commits June 22, 2022 11:13

Update tests/testthat/test-MAEFilteredDataset.R

b0864fd

styler

2038ad7

linter

649fb6d

nikolas-burkoff merged commit a75c135 into main Jun 22, 2022

nikolas-burkoff deleted the 33_filteredData_constructor@main branch June 22, 2022 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FilteredData constructor #49

FilteredData constructor #49

nikolas-burkoff commented Jun 14, 2022 •

edited

Loading

kpagacz commented Jun 14, 2022

This comment was marked as outdated.

gogonzo left a comment

nikolas-burkoff Jun 16, 2022

gogonzo Jun 17, 2022

github-actions bot commented Jun 20, 2022 •

edited

Loading

github-actions bot commented Jun 20, 2022 •

edited

Loading

nikolas-burkoff commented Jun 21, 2022 •

edited

Loading

kpagacz commented Jun 21, 2022 •

edited

Loading

nikolas-burkoff commented Jun 21, 2022

kpagacz Jun 21, 2022

nikolas-burkoff Jun 21, 2022

gogonzo Jun 21, 2022

gogonzo commented Jun 21, 2022 •

edited

Loading

gogonzo commented Jun 21, 2022

gogonzo left a comment

kpagacz left a comment

FilteredData constructor #49

FilteredData constructor #49

Conversation

nikolas-burkoff commented Jun 14, 2022 • edited Loading

kpagacz commented Jun 14, 2022

This comment was marked as outdated.

gogonzo left a comment

Choose a reason for hiding this comment

nikolas-burkoff Jun 16, 2022

Choose a reason for hiding this comment

gogonzo Jun 17, 2022

Choose a reason for hiding this comment

github-actions bot commented Jun 20, 2022 • edited Loading

Unit Tests Summary

github-actions bot commented Jun 20, 2022 • edited Loading

Code Coverage Summary

nikolas-burkoff commented Jun 21, 2022 • edited Loading

kpagacz commented Jun 21, 2022 • edited Loading

nikolas-burkoff commented Jun 21, 2022

kpagacz Jun 21, 2022

Choose a reason for hiding this comment

nikolas-burkoff Jun 21, 2022

Choose a reason for hiding this comment

gogonzo Jun 21, 2022

Choose a reason for hiding this comment

gogonzo commented Jun 21, 2022 • edited Loading

gogonzo commented Jun 21, 2022

gogonzo left a comment

Choose a reason for hiding this comment

kpagacz left a comment

Choose a reason for hiding this comment

nikolas-burkoff commented Jun 14, 2022 •

edited

Loading

github-actions bot commented Jun 20, 2022 •

edited

Loading

github-actions bot commented Jun 20, 2022 •

edited

Loading

nikolas-burkoff commented Jun 21, 2022 •

edited

Loading

kpagacz commented Jun 21, 2022 •

edited

Loading

gogonzo commented Jun 21, 2022 •

edited

Loading