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. Account for the "data_array" property of the "values" attribute of the "dimensions" attribute. Closes #2385 #2387

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

trekonom
Copy link
Contributor

This PR proposes a fix to account for the "data_array" property of the "values" attribute of the "dimensions" attribute used in special traces like "parcoords", "parcats" and "splom" traces thereby closing #2385.

Currently, when an attribute has the "data_array" attribute it gets wrapped in AsIs inside verify_attr. However, this check gets skipped for the dimensions attribute as it is an unnamed list. Additionally, the schema specs for the values attribute are somewhat nested.

To account for that the PR adds some special treatment for dimensions attribute to verify_attr which uses lapply to loop over the items of the dimensions list.

With the proposed fix the examples documented in #2385 work fine, e.g. for "parcats":

library(plotly, warn=FALSE)
#> Loading required package: ggplot2

dd <- data.frame(
  from = "A",
  to = "B"
)

plot_ly(dd) %>%
  add_trace(
    type = "parcats",
    dimensions = list(
      list(values = ~from),
      list(values = ~to)
    )
  )

Created on 2024-08-31 with reprex v2.1.1

…e data_array propery of the "values" attribute. Closes plotly#2385.

* Add tests to check that "values" property has class "AsIs" for "parcoords", "parcats" and "splom" traces.

* Add NEWS entry
R/utils.R Outdated
proposed[[attr]] <- verify_attr(proposed[[attr]], attrSchema, layoutAttr = layoutAttr)
# The "dimensions" attribute requires a special treatment as
# it is an unnamed list and hence will be skipped by `for (attr in names(proposed))
if (attr == "dimensions") {
Copy link
Collaborator

@cpsievert cpsievert Sep 3, 2024

Choose a reason for hiding this comment

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

Seems there is actually a more general problem here that's beyond just dimensions. Some attributes say role: object, but are actually expecting a list of objects. Here is the schema for both dimensions and transforms:

Screenshot 2024-09-03 at 5 09 49 PM Screenshot 2024-09-03 at 5 10 03 PM

So, I think this code should be closer to:

if (identical(role, "object") && is.recursive(proposed[[attr]])) {
      # Some attributes (e.g., dimensions, transforms) are actually 
      # a list of objects (even though they, confusingly, have role: object)
      # In those cases, we actually want to verify each list element
      attr_ <- sub("s$", "", attr)
      is_list_attr <- ("items" %in% names(attrSchema)) && 
        (attr_ %in% names(attrSchema$items))
      if (is_list_attr) {
        proposed[[attr]] <- lapply(proposed[[attr]], function(x) {
          verify_attr(x, attrSchema$items[[attr_]])
        })
      } else {
        proposed[[attr]] <- verify_attr(proposed[[attr]], attrSchema, layoutAttr = layoutAttr)
      }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx for the review and the suggestions. Just added and comitted.

R/utils.R Outdated
Comment on lines 556 to 557
if (attr == "dimensions") {
proposed[[attr]] <- lapply(proposed[[attr]], \(x) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

\(x) was added fairly recently to R, so please use function(x) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙈 Thanks for the reminder. Should have thought of that myself.

NEWS.md Outdated Show resolved Hide resolved
@trekonom trekonom marked this pull request as draft September 6, 2024 10:01
trekonom and others added 2 commits September 6, 2024 12:12
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
@trekonom trekonom marked this pull request as ready for review September 6, 2024 10:14
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.

2 participants