Fix factors crash in tm_g_association

[vedhav]

Closes #645

Changes:
Pass the base class into bivariate_plot_call because we only expect the base class in the downstream function call.

[github-actions]

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  --------------------------------------
R/tm_a_pca.R                    825     825  0.00%    111-1060
R/tm_a_regression.R             777     777  0.00%    153-1034
R/tm_data_table.R               177     177  0.00%    91-317
R/tm_file_viewer.R              172     172  0.00%    42-244
R/tm_front_page.R               129     118  8.53%    66-210
R/tm_g_association.R            332     332  0.00%    130-521
R/tm_g_bivariate.R              665     493  25.86%   192-773, 836, 947, 964, 982, 1002-1024
R/tm_g_distribution.R          1028    1028  0.00%    121-1271
R/tm_g_response.R               351     351  0.00%    139-554
R/tm_g_scatterplot.R            719     719  0.00%    236-1046
R/tm_g_scatterplotmatrix.R      280     261  6.79%    163-460, 514, 528
R/tm_missing_data.R            1054    1054  0.00%    87-1280
R/tm_outliers.R                 976     976  0.00%    132-1238
R/tm_t_crosstable.R             254     254  0.00%    134-426
R/tm_variable_browser.R         820     815  0.61%    86-1314
R/utils.R                       100     100  0.00%    72-300
R/zzz.R                           1       1  0.00%    2
TOTAL                          8660    8453  2.39%

Diff against main

Filename              Stmts    Miss  Cover
------------------  -------  ------  -------
R/tm_g_bivariate.R      +10      -2  +1.44%
TOTAL                   +10      -2  +0.14%

Results for commit: d512862

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

 1 files   5 suites   0s ⏱️
16 tests 16 ✅ 0 💤 0 ❌
49 runs  49 ✅ 0 💤 0 ❌

Results for commit d512862.

♻️ This comment has been updated with latest results.

[chlebowa]

The fix does work but I am not satisfied with the reasoning.
Usually one would use the most junior class by taking ref_class <- class(ANL[[ref_name]])[1L]. The core of the problem is that in this case the junior class, ordered, is not supported. factor, however, is.

Perhaps a better way would be to drop the ordered class. What do you think?

[vedhav]

The core of the problem is that in this case the junior class, ordered, is not supported. factor, however, is.

I disagree that ordered is not supported, well technically it is not supported, but it should be. Looking at the output of the bivariate_plot_call function, I concluded that an ordered column should work fine. The problem is with how we assert the class. For example. This is the output call from the function:

ggplot(ANL) + aes(x = Plant) + geom_bar() + ylab("Frequency") + 
    ggplot2::labs(subtitle = "Plot generated by Association Module", 
        x = "[Plant]") + ggplot2::theme_gray()

And it works fine when the Plant column is either ordered or a factor. Ideally, we should be asserting the class of the column. But, since we do a match.arg I thought the least invasive way to fix the issue is to pass the base class which should work in all scenarios, even in cases where the column uses a derived class from our supported base classes, which is okay for this scenario.

Alternatively, we could pass in the most junior class and add ordered to our supported class. WDYT? As I would like to avoid an unwanted conversion.

[chlebowa]

I disagree that ordered is not supported

😉

we do a match.arg

where?

[vedhav]

I disagree that ordered is not supported

😉

we do a match.arg

where?

😲 When I tested it worked all the time. Can you tell me how to reproduce this?

I'll convert ordered to just factor.

I was talking about the match.arg used here

[chlebowa]

I just ran the example app on your branch but with ref_class <- class(ANL[[ref_name]])[1L].

app code

``` # general data exapmle devtools::load_all() library(teal.widgets)

data <- teal_data()
data <- within(data, {
library(nestcolor)
CO2 <- as.data.frame(CO2)
CO2$Treatment <- seq(Sys.time(), by = 1, length.out = nrow(CO2))
})
datanames(data) <- c("CO2")

app <- init(
data = data,
modules = modules(
tm_g_association(
ref = data_extract_spec(
dataname = "CO2",
select = select_spec(
label = "Select variable:",
choices = variable_choices(data[["CO2"]], c("Plant", "Type", "Treatment")),
selected = "Plant",
fixed = FALSE
)
),
vars = data_extract_spec(
dataname = "CO2",
select = select_spec(
label = "Select variables:",
choices = variable_choices(data[["CO2"]], c("Plant", "Type", "Treatment")),
selected = "Treatment",
multiple = TRUE,
fixed = FALSE
)
),
ggplot2_args = ggplot2_args(
labs = list(subtitle = "Plot generated by Association Module")
)
)
)
)
runApp(app, launch.browser = TRUE)

</details>

[vedhav]

Thanks for the example. I see that this is just because when you made the change to pass the ordered you did not make the required validation changes to support it, Once we do this it should start working fine. I made this change in the commit
77519dd
So I still believe that: ordered is supported in the downstream code, technically it is not supported because we validated it out, but it should be.

[chlebowa]

Forgot to add review comment: requesting changes because of missing full stops in linter exceptions.
Do consider the other suggestions, though 🙂

[chlebowa]

You sure you don't want the more concise syntax? "logical" =, "character" =, "factor" =, "ordered" = "factor"

Actually, the styler will probably wreck it anyway.

[chlebowa]

👍

[vedhav]

Just to confirm, Merging this into main is okay right? Or should I rebase it to pre-release branch and merge it there?

[chlebowa]

Since you started on main, let'e merge there. When pre-release@main is ready to merge, we will decide whether to merge main or keep bug fixes for a patch release.