picks

[gogonzo]

Closes insightsengineering/NEST-roadmap#36

Check also with:

• picks teal.transform#270
• picks teal#1642

In this PR, due to the fact that data_extract_spec is not 1:1 convertible to picks, I propose to have an S3 method in each tm_ to switch between data_extract_spec/picks depending on the input type. Eventually, data_extract_spec variant will be removed completely after deprecation period.

[github-actions]

Unit Tests Summary

  1 files   23 suites   16m 8s ⏱️
155 tests 149 ✅ 5 💤 1 ❌
524 runs  518 ✅ 5 💤 1 ❌

For more details on these failures, see this check.

Results for commit f8b884f.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
shinytest2-tm_a_pca	💚	$157.36$	$-2.37$	$0$	$0$	$0$	$0$
shinytest2-tm_a_regression	💚	$70.59$	$-1.39$	$0$	$0$	$0$	$0$
shinytest2-tm_g_bivariate	💔	$90.72$	$+1.45$	$0$	$0$	$0$	$0$
shinytest2-tm_g_distribution	💚	$69.32$	$-6.92$	$0$	$0$	$0$	$0$
shinytest2-tm_misssing_data	💔	$57.17$	$+6.11$	$+5$	$0$	$0$	$-1$
shinytest2-tm_variable_browser	💔	$65.00$	$+19.41$	$0$	$0$	$-13$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
examples	👶		$+0.01$	example_dot_color_palette_discrete.Rd
examples	👶		$+0.01$	example_dot_make_reactable_columns_call.Rd
examples	👶		$+0.01$	example_dot_plotly_selected_filter_children.Rd
examples	👶		$+0.01$	example_tm_t_reactables.Rd
examples	💀	$0.01$	$-0.01$	example_validate_input.Rd
shinytest2-tm_g_bivariate	💔	$36.31$	$+1.10$	e2e_tm_g_bivariate_Setting_encoding_inputs_produces_outputs_without_validation_errors.
shinytest2-tm_g_distribution	💚	$21.58$	$-5.48$	e2e_tm_g_distribution_Histogram_encoding_inputs_produce_output_without_validation_errors.
shinytest2-tm_g_distribution	💚	$12.86$	$-1.60$	e2e_tm_g_distribution_QQ_plot_encoding_inputs_produce_output_without_validation_errors.
shinytest2-tm_misssing_data	💔	$7.79$	$+6.04$	e2e_tm_missing_data_Validate_functionality_and_UI_response_for_By_Variable_Levels_
shinytest2-tm_variable_browser	💔	$9.57$	$+3.34$	e2e_tm_variable_browser_Selecting_treat_variable_as_factor_changes_the_table_headers.
shinytest2-tm_variable_browser	💔	$11.91$	$+2.56$	e2e_tm_variable_browser_changing_display_density_encoding_doesn_t_show_errors.
shinytest2-tm_variable_browser	💔	$13.26$	$+2.65$	e2e_tm_variable_browser_changing_outlier_definition_encoding_doesn_t_show_errors.
shinytest2-tm_variable_browser	💔	$12.21$	$+8.17$	e2e_tm_variable_browser_changing_plot_setting_encodings_doesn_t_show_errors.
shinytest2-tm_variable_browser	💔	$9.39$	$+1.32$	e2e_tm_variable_browser_content_is_displayed_correctly.
shinytest2-tm_variable_browser	💔	$8.67$	$+1.38$	e2e_tm_variable_browser_selection_of_categorical_variable_has_a_table_with_level_header.

Results for commit 9b0a1a2

♻️ This comment has been updated with latest results.

[llrs-roche]

S3 methods can only dispatch according to one argument. This PR duplicates entire modules for a dispatch on one argument. Isn't it better to have a conversion or a warning on each argument? User might end up mixing the new and the old approach and that could throw off the dispatch mechanism.
Still this would require that some part of the module's code would need to change but I don't think it would be that much. Specially if the full release and removal of the old des object.

[gogonzo]

Isn't it better to have a conversion or a warning on each argument?

It is better, problem is that delayed-filter_spec (with value_choices()) are not convertible to teal_transform_module and thus we would have a breaking change.

I provided this "brute-force" solution to make sure nothing breaks. It is possible to implement filter_spec -> teal_transform_module conversion but it will require significant design intrusion. value_choices(subset = function(data)) where data is a data.frame - this will require delivering data.frame instead of vector from variables to values in order to support value_choices.

Instead of making a design compromise in resolver and determine.values, I would rather vote for changing filter_spec to support subset = function(column) instead.

[llrs-roche]

Isn't it better to have a conversion or a warning on each argument?

It is better, problem is that delayed-filter_spec (with value_choices()) are not convertible to teal_transform_module and thus we would have a breaking change.

Yes, my idea is to raise an error when they are not convertible, and just a deprecation warning when they can be converted.

I provided this "brute-force" solution to make sure nothing breaks. It is possible to implement filter_spec -> teal_transform_module conversion but it will require significant design intrusion. value_choices(subset = function(data)) where data is a data.frame - this will require delivering data.frame instead of vector from variables to values in order to support value_choices.

Instead of making a design compromise in resolver and determine.values, I would rather vote for changing filter_spec to support subset = function(column) instead.

I am never sure of how to ease these transitions between code/designs. There are many tradeoffs and the most important information is hard to estimate: user current usage and user adoption of the future new design. How long we'll we need to keep value_choices and filter_spec working? Even if it is a breaking change it could be with teal.modules.* 1.0.0 release.

[gogonzo]

Yes, my idea is to raise an error when they are not convertible, and just a deprecation warning when they can be converted.

Yup, I'm not sure if soft-deprecation process should start with hard deprecating one of the components. But as a developer I'd be happy to have a single version of the tm_*

How long we'll we need to keep value_choices and filter_spec working? Even if it is a breaking change it could be with teal.modules.* 1.0.0 release.

Theoretically two minor releases. It could be more than one year. It would mean that during the most intensive development (in the early versions of the tool) we would have to deal with extra complication in the design. After the transition period we would remove and keep "raw" picks.

[m7pr]

Suggested change

	rlang,
	rlang (>= 1.0.0),

[m7pr]

IMHO teal.reporter is still needed in Remotes, as it's not released on CRAN?

Suggested change

	insightsengineering/teal.transform@redesign_extraction@main
	insightsengineering/teal.reporter
	insightsengineering/teal.transform@redesign_extraction@main

[m7pr]

Why is that needed? Is it becuase of the usage in tm_missing_data?

[m7pr]

Maybe we can rewrite tm_missing_data to get rid of rlang dependency