Skip to content

Fix trace processing if number of subplots >= 10 #620

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix trace processing if number of subplots >= 10 #620

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions R/layers2traces.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,10 @@ layers2traces <- function(data, prestats_data, layout, p) {
d <- datz[[i]]
# always split on discrete scales, and other geom specific aesthetics that
# can't translate to a single trace
split_by <- c(split_on(d), names(discreteScales))
split_by <- unique(c(split_on(d), names(discreteScales)))
if (length(split_by)>0) split_by <- paste0(split_by, "_plotlyDomain")
# always split on PANEL and domain values (for trace ordering)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alyst I prefer using white space in logical expressions

split_by <- c("PANEL", paste0(split_by, "_plotlyDomain"))
split_by <- c("PANEL", split_by)
# split "this layers" data into a list of data frames
idx <- names(d) %in% split_by
# ensure the factor level orders (which determies traces order)
Expand All @@ -55,16 +56,20 @@ layers2traces <- function(data, prestats_data, layout, p) {
trs <- Map(geom2trace, dl, paramz[i], list(p))
# are we splitting by a discrete scale on this layer?
# if so, set name/legendgroup/showlegend
isDiscrete <- names(d) %in% paste0(names(discreteScales), "_plotlyDomain")
isDiscrete <- names(d) %in% (if (length(discreteScales) > 0) { paste0(names(discreteScales), "_plotlyDomain") } else as.character())
if (length(trs) > 1 && sum(isDiscrete) >= 1) {
Copy link
Collaborator

@cpsievert cpsievert Jun 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alyst I'd prefer

isDiscrete <- names(d) %in% paste0(names(discreteScales), "_plotlyDomain") %||% ""

nms <- names(trs)
# ignore "non-discrete" scales that we've split on
for (w in seq_len(sum(names(d) %in% c("PANEL", split_on(d))))) {
nms <- sub("^[^@%&]@%&", "", nms)
}
nms <- strsplit(nms, "@%&")
nms <- strsplit(names(trs), "@%&", fixed = TRUE)
# ignore "non-discrete" scales that we've split by
n_ignore <- sum(!isDiscrete & (names(d) %in% split_by))
nms <- vapply(nms, function(x) {
if (length(x) > 1) paste0("(", paste0(x, collapse = ","), ")") else x
n_x <- length(x)
if (n_x > n_ignore + 1) {
paste0("(", paste0(x[(n_ignore+1):n_x], collapse = ","), ")")
} else if (n_x > n_ignore) {
x[[n_ignore+1]]
} else {
NA_character_
}
}, character(1))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alyst I don't completely follow this logic. Could you explain a little? Also, this PR will definitely need some tests before it gets merged. Here would be a good example to test #522

Copy link
Contributor Author

@alyst alyst Jun 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing the first ignored scales using sub() in the loop, I've changed the code to directly split the trace ids by "@%&" and then ignore the first n_ignore chunks (non-discrete scales). Then the visible trace names are composed as before: if there's more than one scale left after n_ignore, the trace name is like (a, b), otherwise the remaining scale name is used "as is".
In the original code there was an error in the sub() regex that manifested itself when the length of the non-discrete scale id was more than 1 symbol. So I thought it would be cleaner and safer to avoid preprocessing trace Ids. It should also be faster since we replace multiple regex matching calls with the single strsplit() by the fixed string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the test for #522 definitely makes sense. Do you think of any other tests that should be added before merging this PR?

trs <- Map(function(x, y) {
x$name <- y
Expand Down