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

data instead of datasets #278

Merged
merged 15 commits into from
Sep 8, 2022
Merged

data instead of datasets #278

merged 15 commits into from
Sep 8, 2022

Conversation

mhallal1
Copy link
Collaborator

@mhallal1 mhallal1 commented Aug 25, 2022 •
edited
Loading

closes #247

  • datasets replaced by data in scripts and tests for Filtered and unfiltered data
  • snaps are updated
  • design scripts are not updated
  • PENDING: tests with TESTING_DEPTH = 5 are not passing

@mhallal1 mhallal1 added the core label Aug 25, 2022
@mhallal1 mhallal1 marked this pull request as ready for review August 25, 2022 11:00
@github-actions
Copy link
Contributor

github-actions bot commented Aug 25, 2022 •
edited
Loading

Unit Tests Summary

  1 files  15 suites   19s ⏱️
58 tests 36 ✔️ 22 💤 0 ❌
85 runs  63 ✔️ 22 💤 0 ❌

Results for commit b41b9ea.

♻️ This comment has been updated with latest results.

danielinteractive
danielinteractive reviewed Aug 25, 2022
View reviewed changes
Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

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

Thank you, I wonder if it would be better before merging this to include also passing the deep tests to be sure that everything works

R/experimentSpec.R
mae_name,
name_annotation = "symbol",
sample_vars_as_factors = TRUE,
with_mae_col_data = TRUE) {
assert_string(id)
assert_r6(datasets)
assert_list(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to make this more specific maybe? Otherwise ok as proposed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah this PR is not done yet, in progress

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have created an issue for the deep tests failures: #279

@github-actions
Copy link
Contributor

github-actions bot commented Aug 25, 2022 •
edited
Loading

badge

Code Coverage Summary

Filename              Stmts    Miss  Cover    Missing
------------------  -------  ------  -------  --------------------------------------------------------------------------------------------------------------------------
R/adtteSpec.R           157      11  92.99%   309-313, 359-363, 365
R/assaySpec.R            44      36  18.18%   103-143
R/barplot.R             164     131  20.12%   40-64, 123-252
R/boxplot.R             167     139  16.77%   41-65, 119-254
R/checkmate.R            18       9  50.00%   110-118
R/experimentSpec.R       90      56  37.78%   98, 216-284
R/forestplot.R          195     170  12.82%   60-91, 147-310
R/geneSpec.R            255     136  46.67%   153-168, 297-448
R/km.R                  184     153  16.85%   63-92, 153-301
R/pca.R                 346     263  23.99%   35-55, 163-453
R/quality.R             293     223  23.89%   18-109, 203-425
R/sampleVarSpec.R       221     100  54.75%   296-302, 314-321, 323, 331-342, 344-345, 347, 350, 358-362, 364-379, 384-407, 410-414, 416, 423-433, 435-436, 444, 489-506
R/scatterplot.R         162     132  18.52%   40-64, 122-252
R/utils.R                16       5  68.75%   74-78
R/volcanoplot.R         188     158  15.96%   35-56, 111-274
R/zzz.R                   1       1  0.00%    2
TOTAL                  2501    1723  31.11%

Results for commit: c41686e

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@mhallal1 mhallal1 mentioned this pull request Aug 26, 2022
Closed
mhallal1
mhallal1 commented Aug 29, 2022
View reviewed changes
R/pca.R
Comment on lines -67 to 68
mae <- datasets$get_data(mae_name, filtered = FALSE)
mae <- data[[mae_name]]()
experiment_name_choices <- names(mae)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code was obsolete and not used therefore it was removed.

mhallal1
mhallal1 commented Aug 29, 2022
View reviewed changes
R/experimentSpec.R
Comment on lines -22 to +21
mae <- datasets$get_data(mae_name, filtered = FALSE)
mae <- data[[mae_name]]()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The switch from non-filtered data to filtered data here should not have an impact as we are fetching the names of experiments only.

gogonzo
gogonzo reviewed Aug 31, 2022
View reviewed changes
R/forestplot.R Outdated
@@ -144,10 +145,13 @@ srv_g_forest_tte <- function(id,
plot_height,
plot_width) {
with_reporter <- !missing(reporter) && inherits(reporter, "Reporter")
with_filter <- !missing(filter_panel_api) && inherits(filter_panel_api, "FilterPanelAPI")
Copy link
Contributor

Choose a reason for hiding this comment

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

practically with_filter isn't needed as as module depends on filter_panel_api as experimentSpecServer needs it.

Copy link
Contributor

Choose a reason for hiding this comment

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

so if module depends on filter_panel_api then we should have stop_if_missing(filter_panel_api, "this module requires a filter panel see ....")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with_filter above was replaced with checkmate::assert_class(filter_panel_api, "FilterPanelAPI in all modules using experimentSpecServer.

gogonzo
gogonzo reviewed Aug 31, 2022
View reviewed changes
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

Changes straightforward not affecting functionality of the package. We will try to resolve deep tests problems, but these code changes doesn't seem to be a cause.

nikolas-burkoff
nikolas-burkoff reviewed Sep 1, 2022
View reviewed changes
Copy link
Contributor

@nikolas-burkoff nikolas-burkoff left a comment

Choose a reason for hiding this comment

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

We can remove MockDatasets from tests/helper-data_funs.R

@mhallal1
Copy link
Collaborator Author

mhallal1 commented Sep 1, 2022

MockDatasets

removed.

@gogonzo gogonzo changed the base branch from main to teal_refactor@main September 7, 2022 12:27
@mhallal1 mhallal1 merged commit 15ed0b7 into teal_refactor@main Sep 8, 2022
@mhallal1 mhallal1 deleted the 26_new_chunks@main branch September 8, 2022 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielinteractive danielinteractive danielinteractive left review comments

@nikolas-burkoff nikolas-burkoff nikolas-burkoff left review comments

@gogonzo gogonzo gogonzo approved these changes

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

Successfully merging this pull request may close these issues.

Change modules to use data argument instead of datasets
5 participants
@mhallal1 @danielinteractive @gogonzo @nikolas-burkoff @Polkas