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

Make facet error more informative #3264

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Make facet error more informative #3264

merged 1 commit into from
Nov 14, 2023

Conversation

joelostblom
Copy link
Contributor

Currently if we charts use different data and are trying to be faceted, altair raises ValueError: Facet charts require data to be specified at the top level. I don't think it is immediately clear to everyone what this means, and this PR is an attempt to make the error message more instructive.

Not that there will still be situations like the below that raises the error although the same data is technically used, just respecified as a new variable.

alt.layer(
    alt.Chart(data.movies.url).mark_rule().encode(x='IMDB_Rating:Q'),
    alt.Chart(data.movies.url).mark_rule().encode(x='IMDB_Rating:Q')
).facet(
    'Major_Genre:N'
)

I am not sure if altair should be capable of figuring out that this is the same as

source = data.movies.url
alt.layer(
    alt.Chart(source).mark_rule().encode(x='IMDB_Rating:Q'),
    alt.Chart(source).mark_rule().encode(x='IMDB_Rating:Q')
).facet(
    'Major_Genre:N'
)

and avoid the error, maybe those comparisons will be too complex for large data frames and non-portable across dataframe implementations (although many have some type of equality test built-in like pandas).

@mattijn
Copy link
Contributor

mattijn commented Nov 14, 2023

Could these datasets within a facet be consolidated as is done for other data?

See this function: https://github.com/altair-viz/altair/blob/main/altair/vegalite/v5/api.py#L63

@joelostblom
Copy link
Contributor Author

Yeah I think that is a good thing to explore, but maybe in a separate PR? I opened #3265

@mattijn
Copy link
Contributor

mattijn commented Nov 14, 2023

Thanks!

@mattijn mattijn merged commit bf52ed1 into main Nov 14, 2023
20 checks passed
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