Accept logicals for truth (and estimate) inputs

[tripartio]

First, I am fully aware that this request has already been made and declined (#450). However, I hope that this request is different because it includes a PR (#544) with a resolution that I hope works.

TLDR: instead of rejecting logical inputs for truth or estimates as errors, just internally convert them to factors and then everything else works fine, with no headaches for yardstick developers.

This request started out as a bug report on hardhat. It evolved into an explanation for why tidymodels and yardstick don't support logical outcomes for binary prediction. Please excuse me for the lengthy quotation, but I think it's important to state here the motivation for my PR.

1. The real-world data has qualitative meaning, so users should be forced to encode the outcomes according to their qualitative meaning.
2. Since binary data has intrinsic qualitative meaning, it should be encoded as factors, which is the native R datatype for encoding qualitative meaning, just like multinomial outcomes.
3. Binary data lacks labels to distinguish the FALSE from the TRUE cases.

For the first point, I know you did not use the word "forced", but that is how I understood your point. And this is where I think I disagree. On one hand, I agree that as a package author, I should be disciplined to follow conventions such as tidymodels. On the other hand, I think that my packages should be sufficiently flexible to accept all reasonable kinds of input data that users provide. I consider it quite unreasonable to reject logical outcomes as "unreasonable" or "illegitimate" on the package level. logical is such a fundamental data format (for predictors and outcomes alike). It is the most natural and intuitive format for binary outcomes. I definitely think that any binary prediction modelling package should be able to cleanly and naturally handle logical outcomes without forcing the user to encode them as factors.

For the second and third points, I understand the logic, but again, I don't think such logic should be imposed on users. The resolution to me is very natural: the two levels should be labelled as "FALSE" and "TRUE". I don't understand why that would be complicated. If users want something better than that, then they should follow your advice and encode the outcomes as factors. But that should be their choice.

One important point that you implied here but made explicit in your 2012 "rant" is that it is unclear which of the two binary cases should be considered the primary or "positive" class. However, here I think your argument is self-defeating (unless I misunderstand it). This is only a problem when your advice is followed and binary outcomes are encoded as factors--indeed, then there is no natural choice of the first or the second level as the positive case. … However, when the binary outcome is kept logical, then there is no issue. The clear and natural choice is that TRUE is the positive case. There's no ambiguity there. …

My point here is not to convince you or anyone else to change the tidymodels conventions, but my point is hopefully to convince you to give first-class support for "binary outcomes as logical". … I hope that my arguments are sufficient to respect the choice to support such functionality for package designers who disagree with this controversial point, rather than shutting out much of the fantastic tidymodels infrastructure from us just because of this point of disagreement.

So, as a proposed resolution to this issue, my PR does the following: instead of rejecting logical inputs as errors, it converts them to factors (and sets TRUE to be the event_level if applicable). This solution is very simple and works elegantly in the PR. Its only inconvenience is that it required adding the following code (or versions thereof) in almost 30 places throughout the package, right before any call to check_class_metric() or check_prob_metric() that check for valid inputs:

  if (is.logical(truth)) {
    event_level <- "second"  # TRUE is second level of levels(factor(truth))
    truth <- factor(truth)
    estimate <- factor(estimate)
  }

This is done in the PR (with a couple of tests) and it works great. Crucially, it doesn't mess with any code that assumes working with factors, so it should be easy to maintain and extend for future functionality. Additionally, since this converts a previous error condition to an accepted condition, it should not break any working code anywhere downstream or in users' scripts.

Could you please accept it? Again, the goal of this request is so that developers like me who believe that that binary outcomes are more naturally logicals than factors can still benefit from the fantastic yardstick infrastructure., without creating any development or maintenance headaches for the yardstick developers.

If this solution works for yardstick, it can hopefully be extended to other tidymodels packages as relevant.

[EmilHvitfeldt]

Hello @tripartio!

I'm still not convinced that this is a good idea for {yardstick}. While the changes works in the PR it introduces some sharp edges. Example below:

Passing in a logical and character with return a result or error depending on the order. Both of these are bad as ideally they contain different types of input. In yardstick we are quite strict with the input as deviations between truth and predicted often signal that something went wrong.

library(yardstick)

accuracy_vec(
  two_class_example$truth == 'Class1',
  c("TRUE", "FALSE")[as.integer(two_class_example$predicted)]
)
#> [1] 0.838

accuracy_vec(
  c("TRUE", "FALSE")[as.integer(two_class_example$truth)],
  two_class_example$predicted == 'Class1'
)
#> Error in `accuracy_vec()`:
#> ! `truth` should be a factor, not a a character vector.

Overwriting the event_level also isn't ideal as it renders the argument void without a warning or documentation. It also decreases its use as you no longer can compare the metric value for "first" and "second" when using logicals.

I would like to point out that you can still use {yardstick} in your packages, but it will be up to use to do the conversion to factors in that package instead of inside {yardstick}.

[tripartio]

Passing in a logical and character with return a result or error depending on the order. Both of these are bad as ideally they contain different types of input. In yardstick we are quite strict with the input as deviations between truth and predicted often signal that something went wrong.

library(yardstick)

accuracy_vec(
two_class_example$truth == 'Class1',
c("TRUE", "FALSE")[as.integer(two_class_example$predicted)]
)
#> [1] 0.838

accuracy_vec(
c("TRUE", "FALSE")[as.integer(two_class_example$truth)],
two_class_example$predicted == 'Class1'
)
#> Error in accuracy_vec():
#> ! truth should be a factor, not a a character vector.

Sorry about that; it was a bug in the PR. I've fixed it. Now, both cases produce errors: I fully agree that character inputs should always produce errors.

Here is the corrected base code:

if (is.logical(truth)) {
   event_level <- "second"  # TRUE is second level of levels(factor(truth))
   truth <- factor(truth)
   # bug: estimate <- factor(estimate)
   # correction:
   estimate <- if(is.logical(estimate)) factor(estimate) else estimate
 }

The fix now correctly rejects all character inputs:

accuracy_vec(
  two_class_example$truth == 'Class1',
  c("TRUE", "FALSE")[as.integer(two_class_example$predicted)]
)
# Error in `accuracy_vec()`:
# ! `estimate` should be a factor, not a a character vector.

accuracy_vec(
  c("TRUE", "FALSE")[as.integer(two_class_example$truth)],
  two_class_example$predicted == 'Class1'
)
# Error in `accuracy_vec()`:
# ! `truth` should be a factor, not a a character vector.

I've added tests that verify this correction.

[tripartio]

I'm still not convinced that this is a good idea for {yardstick}. While the changes works in the PR it introduces some sharp edges. Example below:

Apart from the bug you uncovered (thanks!), which has now been resolved, I don't see the "sharp edges" that are introduced. Please note that the bug is not directly related to the issue at hand (other than the fact that it was introduced in trying to support logicals)--I am only asking for logicals not to be rejected as errors. Character inputs and anything else should still be errors.

Overwriting the event_level also isn't ideal as it renders the argument void without a warning or documentation. It also decreases its use as you no longer can compare the metric value for "first" and "second" when using logicals.

I'm sorry, but I don't completely follow your logic here. One of the key arguments in favour of using logicals is that they render the event_level superfluous. TRUE is always the natural, intuitive level for a logical; setting it to FALSE would be nonsensical. There is no need to specify event_level any further; that variable is only needed because it solves a new problem created by your decision to represent binary outcomes as factors rather than logicals. The only reason I set event_level = "second" is so that all yardstick code that depends on factors works fine with no further changes. Again, my goal is not to change the tidymodels decision of binary-as-factors; it is simply to also accept binary-as-logicals as a legitimate non-error with minimal disruption to your existing code framework.

I would definitely like this new proposed functionality to be officially documented, but I did not dare request that since I respect the tidymodels decision to favour factors. So, I am only being so bold as to propose that logicals be silently accepted rather than being rejected as errors. Perhaps this could be implemented as an undocumented experimental feature for now (only mentioned in the changelog), but if after some use in the wild there is no problem, it could become officially documented in other documentation more easily visible to users.

I repeat a point I made earlier (notwithstanding the now-fixed character bug): this PR implementation should not break any code existing in the wild because it now accepts as a non-error certain conditions (logical inputs) that were previously rejected as errors. So, I believe this would be a very safe experimental feature to add.

[tripartio]

To make sure that there are no negative downstream effects of this PR, I ran a full reverse dependency check (2 hours on my computer), and the result is BROKEN: 0. So, all looks good as expected. Here are the full results:

revdepcheck::revdep_check()
# ── INIT ─────────────────────────────────────────────────────────────────────── Computing revdeps ──
# ── INSTALL ─────────────────────────────────────────────────────────────────────────── 2 versions ──
# Installing CRAN version of yardstick
# Warning: package ‘yardstick’ is in use and will not be installed
# Warning: package ‘yardstick’ is in use and will not be installed
# Installing DEV version of yardstick
# Installing 18 packages: rlang, glue, cli, lifecycle, vctrs, utf8, pkgconfig, pillar, magrittr, fansi, withr, tibble, sparsevctrs, tidyselect, R6, generics, hardhat, dplyr
# package ‘rlang’ successfully unpacked and MD5 sums checked
# package ‘glue’ successfully unpacked and MD5 sums checked
# package ‘cli’ successfully unpacked and MD5 sums checked
# package ‘lifecycle’ successfully unpacked and MD5 sums checked
# package ‘vctrs’ successfully unpacked and MD5 sums checked
# package ‘utf8’ successfully unpacked and MD5 sums checked
# package ‘pkgconfig’ successfully unpacked and MD5 sums checked
# package ‘pillar’ successfully unpacked and MD5 sums checked
# package ‘magrittr’ successfully unpacked and MD5 sums checked
# package ‘fansi’ successfully unpacked and MD5 sums checked
# package ‘withr’ successfully unpacked and MD5 sums checked
# package ‘tibble’ successfully unpacked and MD5 sums checked
# package ‘sparsevctrs’ successfully unpacked and MD5 sums checked
# package ‘tidyselect’ successfully unpacked and MD5 sums checked
# package ‘R6’ successfully unpacked and MD5 sums checked
# package ‘generics’ successfully unpacked and MD5 sums checked
# package ‘hardhat’ successfully unpacked and MD5 sums checked
# package ‘dplyr’ successfully unpacked and MD5 sums checked
# Warning: package ‘yardstick’ is in use and will not be installed
# Warning: package ‘yardstick’ is in use and will not be installed
# ── CHECK ──────────────────────────────────────────────────────────────────────────── 42 packages ──
# ✔ adjROC 0.3                             ── E: 1     | W: 0     | N: 0                              
# ✔ autostats 0.4.1                        ── E: 1     | W: 0     | N: 0                              
# ✔ baguette 1.1.0                         ── E: 0     | W: 1     | N: 1                              
# ✔ BayesERtools 0.2.1                     ── E: 0     | W: 0     | N: 1                              
# ✔ brulee 0.5.0                           ── E: 0     | W: 0     | N: 1                              
# ✔ diceR 3.0.0                            ── E: 1     | W: 0     | N: 0                              
# ✔ EZtune 3.1.1                           ── E: 1     | W: 1     | N: 2                              
# ✔ fastml 0.5.0                           ── E: 1     | W: 0     | N: 0                              
# ✔ fastTS 1.0.2                           ── E: 1     | W: 0     | N: 0                              
# ✔ FCPS 1.3.4                             ── E: 0     | W: 0     | N: 2                              
# I finetune 1.2.1                         ── E: 1     | W: 0     | N: 1                              
# ✔ forestecology 0.2.0                    ── E: 1     | W: 0     | N: 0                              
# ✔ garma 0.9.24                           ── E: 0     | W: 1     | N: 1                              
# ✔ healthyR.ai 0.1.1                      ── E: 1     | W: 0     | N: 0                              
# ✔ ldmppr 1.0.4                           ── E: 1     | W: 0     | N: 0                              
# ✔ modeltime 1.3.1                        ── E: 1     | W: 0     | N: 0                              
# ✔ modeltime.ensemble 1.0.4               ── E: 1     | W: 0     | N: 0                              
# ✔ modeltime.resample 0.2.3               ── E: 1     | W: 0     | N: 0                              
# ✔ postcard 1.0.0                         ── E: 1     | W: 0     | N: 0                              
# ✔ probably 1.1.0                         ── E: 1     | W: 0     | N: 0                              
# ✔ rTwig 1.4.0                            ── E: 0     | W: 1     | N: 1                              
# ✔ sae.projection 0.1.3                   ── E: 1     | W: 0     | N: 0                              
# ✔ shapr 1.0.4                            ── E: 0     | W: 0     | N: 3                              
# ✔ shinymodels 0.1.1                      ── E: 1     | W: 0     | N: 0                              
# ✔ spatialsample 0.6.0                    ── E: 0     | W: 1     | N: 1                              
# ✔ stacks 1.1.1                           ── E: 1     | W: 2     | N: 2                              
# I tabnet 0.7.0                           ── E: 1     | W: 0     | N: 1                              
# ✔ text 1.5                               ── E: 1     | W: 0     | N: 0                              
# ✔ TidyConsultant 0.1.1                   ── E: 0     | W: 1     | N: 2                              
# ✔ tidydann 1.0.1                         ── E: 1     | W: 0     | N: 1                              
# ✔ tidyfit 0.7.4                          ── E: 1     | W: 0     | N: 0                              
# ✔ tidymodels 1.3.0                       ── E: 1     | W: 0     | N: 0                              
# ✔ tidyposterior 1.0.1                    ── E: 1     | W: 0     | N: 1                               
# ✔ tidysdm 1.0.0                          ── E: 1     | W: 0     | N: 0                               
# ✔ tidytof 1.0.0                          ── E: 1     | W: 0     | N: 0                               
# ✔ treeheatr 0.2.1                        ── E: 1     | W: 0     | N: 0                               
# ✔ trendeval 0.1.1                        ── E: 1     | W: 0     | N: 0                               
# ✔ tune 1.3.0                             ── E: 1     | W: 0     | N: 0                               
# ✔ vetiver 0.2.5                          ── E: 1     | W: 0     | N: 2                               
# ✔ vip 0.4.1                              ── E: 1     | W: 0     | N: 0                               
# ✔ waywiser 0.6.3                         ── E: 1     | W: 0     | N: 0                               
# I workflowsets 1.1.1                     ── E: 1     | W: 0     | N: 1                               
# OK: 42                                                                                            
# 
# BROKEN: 0
# Total time: 2 hours
# ── REPORT ───────────────────────────────────────────────────────────────────────────────────────────
# Writing summary to 'revdep/README.md'
# Writing problems to 'revdep/problems.md'
# Writing failures to 'revdep/failures.md'
# Writing CRAN report to 'revdep/cran.md'
# Warning message:
#   In system2("quarto", "-V", stdout = TRUE, env = paste0("TMPDIR=",  :
#                                                            running command '"quarto" TMPDIR=C:/Users/chitu/AppData/Local/Temp/RtmpqY8vMS/filea45473504d28 -V' had status 1
#                                                          

[tripartio]

If there are any other important checks to be run, please let me know.

[tripartio]

I assume that this final decision from tidymodels/hardhat#289 (comment) applies here:

There is a lot more to consider than yardstick. A lot of tidymodels is about smoothing out the disparate APIs and arguments across R packages. We have a lot of code that translates an R factor to integers, zero-based integers, characters, etc. because that's how the package author wanted it. We'd also have to change all of that code. There are other complications too, and they happen across the board.

Is there a reason that you cannot convert to a factor before your computations? Given the risk-benefit ratio, that seems like a sound option.

I know you really want this feature. Respectfully, we think that the overhead outweighs the need so we won't make changes to this effect. Sorry.

So, I repeat my response from there:

Thanks for your consideration. Yes, I would have liked to see support for logical outcomes incorporated into tidymodels packages, but if the decision continues to be not to support this, then I will try to work around it by converting my logical outcomes to factors whenever I want to use tidymodels.