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

Surprising but expected error with once = TRUE when traced fn is copied #59

Closed
yjunechoe opened this issue Nov 1, 2021 · 3 comments
Closed
Labels
bug Something isn't working
Milestone

Comments

@yjunechoe
Copy link
Owner

reprex:

library(ggtrace)
library(ggplot2)

ellipse_plot <- ggplot(palmerpenguins::penguins, aes(bill_length_mm, bill_depth_mm)) +
  stat_ellipse(
    aes(color = species, fill = after_scale(alpha(color, 0.5))),
    geom = "polygon"
  ) +
  geom_point()
> ggtrace(GeomPolygon$draw_key, -1, verbose = FALSE)
`GeomPolygon$draw_key` now being traced.
> ellipse_plot
Triggering trace on `GeomPolygon$draw_key`
Untracing `GeomPolygon$draw_key` on exit.
Triggering trace on `GeomPolygon$draw_key`
Error in methods::.TraceWithMethods(what = "draw_key", where = <environment>,  : 
  could not find function "draw_key"
In addition: Warning messages:
1: Removed 2 rows containing non-finite values (stat_ellipse). 
2: Removed 2 rows containing missing values (geom_point). 

The error is traceable to here: the traced method is copied and wrapped in the internal helper function draw_key inside ggplot2:::guide_gengrob.legend where legend drawing is resolved, so untracing the original method on exit does not spill over. So that's why you get the second trigger on the method and then the error that the traced function is not found (because the <functionWithTrace> wrapper has been removed in the untrace)

> ggbody(ggplot2:::guide_gengrob.legend)[[36]]
draw_key <- function(i) {
    bg <- element_render(theme, "legend.key")
    keys <- lapply(guide$geoms, function(g) {
        g$draw_key(g$data[i, ], g$params, key_size)
    })
    c(list(bg), keys)
}
> ggbody(ggplot2:::guide_gengrob.legend)[[37]]
grob.keys <- unlist(lapply(seq_len(nbreak), draw_key), recursive = FALSE)

This error doesn't replicate in other contexts where such copies aren't being made. Ex: the boxplot from the readme. Tracing "compute_group" once just returns data from the first boxplot without erroring, which is the expected behavior:

> boxplot_plot <- ggplot(diamonds[1:500,], aes(cut, depth)) +
+     geom_boxplot()
> ggtrace(StatBoxplot$compute_group, -1)
`StatBoxplot$compute_group` now being traced.
> boxplot_plot
Triggering trace on `StatBoxplot$compute_group`

[Step 19]> flip_data(df, flipped_aes)
  ymin lower middle  upper ymax outliers notchupper notchlower x width relvarwidth
1 53.1 58.85   64.8 65.775 68.1             66.9458    62.6542 1  0.75     5.09902
  flipped_aes
1       FALSE

Call `last_ggtrace()` to get the trace dump.
Untracing `StatBoxplot$compute_group` on exit.
@yjunechoe yjunechoe added documentation Improvements or additions to documentation wontfix This will not be worked on labels Nov 1, 2021
@yjunechoe
Copy link
Owner Author

yjunechoe commented Nov 1, 2021

This behavior should be documented somewhere with the recommendation that you should trace with once = FALSE if copies of the method are being made internally + it's ran multiple times in a single plot. It's fine that the copy also gets the permanent trace because it'll just be garbage collected (assuming it's an internal helper function).

If you make a copy of a traced method that is bound somewhere, that complicates things. ATM can't think of a non-deliberate case where this might happen (maybe in training scales/layout? but those aren't stateful across plots so maybe its not a problem?)

@yjunechoe
Copy link
Owner Author

Actually maybe there is a way to make this fail gracefully. Given that most error points for trace() are handled inside ggtrace() before the actual call to trace(), we should be able to tell beforehand whether the function being traced does actually exist.

If we get a could not find function error but we know that the initial trace was successful (i.e., the fn does exist), can we take that as evidence that the issue is about the trace being triggered on the copy of the traced fn with once = TRUE? Maybe we can catch for this error message and return a more informative msg about trace being triggered on a copy?

We also need to check where the error comes from though... if it's .doTrace() we can't tryCatch() it

@yjunechoe yjunechoe added questioning and removed documentation Improvements or additions to documentation wontfix This will not be worked on labels Feb 3, 2022
@yjunechoe yjunechoe added this to the v0.5 milestone Feb 3, 2022
@yjunechoe
Copy link
Owner Author

This might be possible if tracer function is modified such that when once = TRUE, you evaluate the exprs and then immediately untrace. The active evaluation of .is_traced() condition at top prevents the trace from being triggered later.

ggtrace/R/one-offs.R

Lines 35 to 40 in 73df8ec

tracer = rlang::expr({
if (rlang::eval_bare(.is_traced(!!what, !!where))) {
print(rlang::trace_back(!!!params))
gguntrace(!!method_quo)
}
}),

Check if this accidentally interferes with multiple traces within the same execution env though

@yjunechoe yjunechoe added bug Something isn't working and removed questioning labels Mar 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant