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.

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

fix baseline plotting #40

Merged
merged 2 commits into from
Oct 18, 2024
Merged

fix baseline plotting #40

merged 2 commits into from
Oct 18, 2024

Conversation

RaphaelS1
Copy link
Contributor

Adds two sided CD line for baseline plots and changes lty type for baseline (see images)
Screenshot 2024-02-24 at 09 49 22
Screenshot 2024-02-24 at 09 49 35

@jemus42
Copy link
Member

jemus42 commented Feb 24, 2024

Gave it a quick go for the style = 1, there seems to be a bug I'll try sorting out

Error in `$<-.data.frame`:
! replacement has 0 rows, data has 17
Backtrace:
 1. global plot_results(...)
 3. mlr3benchmark:::autoplot.BenchmarkAggr(...)
 4. mlr3benchmark:::.plot_critdiff_1(...)
 6. base::`$<-.data.frame`(`*tmp*`, "baseline", value = `<lgl>`)

@jemus42
Copy link
Member

jemus42 commented Feb 24, 2024

Ah right I don't have access here -- anyway, at the top of .plot_critdiff_X added the following workaround:

  if (!is.null(cd$data$baseline)) {
    cd$data$baseline = as.logical(cd$data$baseline)
  } else {
    cd$data$baseline = FALSE
  }

Because obj$.__enclos_env__$private$.crit_differences (is it odd to call a private method like that btw?) does not necessarily return cd$data with a baseline variable, and as.logical then fails on NULL.

The proper workaround would probably be to have obj$.__enclos_env__$private$.crit_differences always include a baseline variable that is all 0 (or FALSE, which seems to be needed anyway?), such that further plot functions can reliably test for it.

@jemus42
Copy link
Member

jemus42 commented Mar 12, 2024

Can this be merged?

@jemus42
Copy link
Member

jemus42 commented Oct 8, 2024

Ping?

@RaphaelS1
Copy link
Contributor Author

if you're pinging me, i have no idea? from our conversation it looks like probably but IDK?

@jemus42
Copy link
Member

jemus42 commented Oct 10, 2024

I guess I should be pinging @sebffischer to merge this 😅

@sebffischer sebffischer merged commit c854b3a into main Oct 18, 2024
4 checks passed
@sebffischer sebffischer deleted the fix_bd_plot branch October 18, 2024 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants