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

Lifting parameters to the top level #2702

Merged
merged 5 commits into from
Oct 29, 2022
Merged

Lifting parameters to the top level #2702

merged 5 commits into from
Oct 29, 2022

Conversation

ChristopherDavisUCI
Copy link
Contributor

@ChristopherDavisUCI ChristopherDavisUCI commented Oct 25, 2022

This should have the same functionality as #2684, just with a cleaner commit history. The only change in functionality from #2684 is that layer is allowed to be mixed with row or column in Repeat Charts.

The main goal of this PR is to improve the behavior of parameters in multi-view charts (whether added at the top level or at the level of the constituent charts).

Thanks to @mattijn especially for the dozens of comments and examples.

In multi-view charts, all parameters will live on the top level.  When necessary, the parameters will specify which constituent charts they should be attached to.
@ChristopherDavisUCI
Copy link
Contributor Author

The following example should be working on this branch.

import altair as alt
from vega_datasets import data

source = data.movies.url

alt.Chart(source).mark_line().encode(
    x=alt.X(alt.repeat("column"), bin=True),
    # x=alt.X("IMDB_Rating:Q", bin=True),
    y=alt.Y(
        alt.repeat("layer"), aggregate="mean", title="Mean of US and Worldwide Gross"
    ),
    color=alt.ColorDatum(alt.repeat("layer")),
).repeat(
    layer=["US_Gross", "Worldwide_Gross"],
    column=["IMDB_Rating", "Rotten_Tomatoes_Rating"],
)

@ChristopherDavisUCI
Copy link
Contributor Author

@jakevdp @joelostblom @mattijn I think what's here is pretty much finalized. Are there any changes you'd like to see? For now I'm planning to switch focus to #2592 (comment)

Thanks again @mattijn for all the help.

Same changes as #2591
@ChristopherDavisUCI
Copy link
Contributor Author

I added the changes from #2591 to this PR. Thanks @joelostblom for noticing #2591 had been mistakenly deleted!

Aligning versions with those used by altair_viewer.
@mattijn
Copy link
Contributor

mattijn commented Oct 29, 2022

Thanks! All tests on Github Actions are passing.
The VL-specification are created like we anticipated for the compound charts. Still a few outstanding issues for rendering on the VL-side. Just listing for reference:

Merging this🥳

@mattijn mattijn merged commit 066be25 into vega:master Oct 29, 2022
@ChristopherDavisUCI ChristopherDavisUCI deleted the liftparams2 branch October 29, 2022 16:47
@joelostblom
Copy link
Contributor

Awesome seeing this merged, thank you both for your hard work on it!

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