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.

Sign up for GitHub

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

Jump to bottom

Accept functions #624

Closed
wants to merge 3 commits into from
Closed

Accept functions #624

wants to merge 3 commits into from

Conversation

gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Oct 22, 2024

Part of insightsengineering/teal#1352

Ignores other datasets than data.frame and MultiAssayExperiment. Unfilterable datasets shouldn't be displayed in the filter-panel as they will be printed independently in the teal-module_data_summary.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 22, 2024 •
edited
Loading

Unit Tests Summary

  1 files   29 suites   23s ⏱️
361 tests 361 ✅ 0 💤 0 ❌
827 runs  827 ✅ 0 💤 0 ❌

Results for commit 242b063.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 22, 2024 •
edited
Loading

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
init_filtered_data 💀 $0.01$ $-0.01$ init_filtered_data.default_asserts_join_keys_is_join_keys_
init_filtered_data 💀 $0.01$ $-0.01$ init_filtered_data.default_asserts_x_has_unique_names
init_filtered_data 👶 $+0.01$ init_filtered_data_asserts_join_keys_is_join_keys_
init_filtered_data 👶 $+0.01$ init_filtered_data_asserts_x_has_unique_names
init_filtered_data 👶 $+0.05$ init_filtered_data_ignores_datasets_if_they_are_of_different_class_than_data.frame_and_MAE

Results for commit fec2f2c

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link
Contributor

badge

Code Coverage Summary

Filename                        Stmts    Miss  Cover    Missing
----------------------------  -------  ------  -------  -----------------------------------------------------------------------------------------------------------------------
R/calls_combine_by.R                7       0  100.00%
R/choices_labeled.R                49      14  71.43%   25, 36, 41, 51-56, 68, 72-76
R/count_labels.R                  102       0  100.00%
R/filter_panel_api.R               27       1  96.30%   132
R/FilteredData-utils.R             71      25  64.79%   21-24, 27-30, 55-60, 156, 178-187
R/FilteredData.R                  540     186  65.56%   110, 184, 326, 398, 503-511, 534, 553-596, 616-619, 660, 697, 718-750, 773-775, 778-792, 796-806, 809-852, 909, 921-943
R/FilteredDataset-utils.R          23       1  95.65%   125
R/FilteredDataset.R               232      72  68.97%   48, 147, 179, 206-276, 379
R/FilteredDatasetDataframe.R      120       8  93.33%   87, 148, 158, 233-237
R/FilteredDatasetDefault.R         18       4  77.78%   104-117
R/FilteredDatasetMAE.R            133      37  72.18%   56, 117-122, 159-164, 168-169, 187-209
R/FilterPanelAPI.R                 10       0  100.00%
R/FilterState-utils.R             101       2  98.02%   264, 294
R/FilterState.R                   363      61  83.20%   89, 213, 231-235, 242-243, 257-258, 264-265, 321, 323, 325, 377, 420, 641, 684-707, 717-736, 771-777, 786-792
R/FilterStateChoices.R            352     109  69.03%   301, 368-375, 379-396, 423-426, 439-450, 462-470, 474-503, 523-526, 529-532, 542-567, 578, 583, 594
R/FilterStateDate.R               213     127  40.38%   230, 282-437
R/FilterStateDatettime.R          307     197  35.83%   266, 318-547
R/FilterStateEmpty.R               53      31  41.51%   89, 99-104, 117, 129-169
R/FilterStateExpr.R                83      68  18.07%   167-280
R/FilterStateLogical.R            194     142  26.80%   136, 158, 218, 222-404
R/FilterStateRange.R              410     104  74.63%   262, 384, 517-521, 524-534, 537, 548-554, 565-577, 581-591, 595-597, 610-636, 651, 654, 668-685, 720-725, 735-737
R/FilterStates-utils.R             70       7  90.00%   108, 127, 188-194
R/FilterStates.R                  361      31  91.41%   76-80, 187, 309-318, 403-406, 449, 487, 550-554, 599, 719-722
R/FilterStatesDF.R                  5       0  100.00%
R/FilterStatesMAE.R                10       1  90.00%   40
R/FilterStatesMatrix.R              3       0  100.00%
R/FilterStatesSE.R                216      62  71.30%   36, 71-73, 83-85, 108-115, 123-130, 206, 231, 250-268, 276-294, 302
R/include_css_js.R                  5       5  0.00%    12-16
R/teal_slice.R                    107       4  96.26%   131, 195-196, 206
R/teal_slices.R                    84       5  94.05%   150-155
R/test_utils.R                     21       0  100.00%
R/utils.R                          20       0  100.00%
R/variable_types.R                 15       1  93.33%   48
R/zzz.R                            17      17  0.00%    3-47
TOTAL                            4342    1322  69.55%

Diff against main

Filename                  Stmts    Miss  Cover
----------------------  -------  ------  -------
R/FilteredData-utils.R       +3       0  +1.55%
TOTAL                        +3       0  +0.02%

Results for commit: 242b063

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@gogonzo gogonzo changed the title 1352 accept function@main Accept functions Oct 22, 2024
@gogonzo gogonzo added the core label Oct 22, 2024
@gogonzo gogonzo mentioned this pull request Oct 22, 2024
Closed
@gogonzo gogonzo linked an issue Oct 22, 2024 that may be closed by this pull request
[Bug]: functions in data raise warnings insightsengineering/teal#1352
Closed
3 tasks
chlebowa
chlebowa reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@chlebowa chlebowa left a comment

Choose a reason for hiding this comment

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

When we made the filter panel "data agnostic" it was considered an overdue success (I cannot find the exact comment that celebrated it).
This change reverses that, the filter panel becomes limited to strictly specified classes again.

@gogonzo
Copy link
Contributor Author

gogonzo commented Oct 22, 2024

When we made the filter panel "data agnostic" it was considered an overdue success (I cannot find the exact comment that celebrated it). This change reverses that, the filter panel becomes limited to strictly specified classes again.

It doesn't revert that state which was a failure when unsupported dataset was specified. But thanks for mentioning this, you reminded me that there is "DefaultFilteredDataset" class to remove ;)

@gogonzo gogonzo marked this pull request as draft October 22, 2024 13:59
chlebowa
chlebowa reviewed Oct 23, 2024
View reviewed changes
R/FilteredDataset-utils.R
dataname = dataname,
label = label
)
NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

How about an error message?

pawelru
pawelru reviewed Oct 23, 2024
View reviewed changes
R/FilteredData-utils.R
Comment on lines +32 to +35
FilteredData$new(
Filter(function(obj) inherits(obj, c("data.frame", "MultiAssayExperiment")), x),
join_keys = join_keys
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I echo @chlebowa comment here. I think this should be delegated lower into the dataset constructor. Conceptually - a function (or anything like that) cannot become a filtered dataset.

@gogonzo
Copy link
Contributor Author

gogonzo commented Oct 24, 2024

Closing this PR as changes in teal.slice are not needed. PR in teal alone fixes the problem.

@gogonzo gogonzo closed this Oct 24, 2024
@gogonzo gogonzo deleted the 1352_accept_function@main branch October 24, 2024 06:41
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chlebowa chlebowa chlebowa left review comments

@pawelru pawelru pawelru left review comments

Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: functions in data raise warnings
3 participants
@gogonzo @pawelru @chlebowa