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

WIP: update to Vega-Lite 5 #2516

Closed
wants to merge 51 commits into from
Closed

WIP: update to Vega-Lite 5 #2516

wants to merge 51 commits into from

Conversation

ChristopherDavisUCI
Copy link
Contributor

(Draft version only!)

Surprisingly the biggest obstacle so far hasn't been params but a change to layer. I think in the newest Vega-Lite schema, charts in a layer are not allowed to specify height or width, which seems to break many Altair examples. Here is a minimal example that doesn't work:

no_data = pd.DataFrame()

c = alt.Chart(no_data).mark_circle().properties(
    width=100
)

c+c

I don't see a good way to deal with that. Do you have a suggestion?

I've read the list of "breaking changes" for the Vega-Lite 5.0.0 release and don't see anything that seems related to this, so it does make me wonder if maybe I misunderstand the cause of the problem.

Other things:

  • This is silly but I don't understand why it lists so many commits. Most of those old commits were already merged. Is there an easy way to fix that? If I were left to my own, I would just create a new branch and move over the changed files (there are only a few I made changes in).
  • I usually try some tests by running things in Jupyter notebook, but since making the change to Vega-Lite 5 that hasn't worked for me. Instead I get the following message in red the first time I try to display a chart, and then subsequent times I just get a blank response: Error loading script: Script error for "vega-util", needed by: vega-lite http://requirejs.org/docs/errors.html#scripterror It does work in Jupyter Lab.
  • Some of the code changes have been experiments trying to learn how the old selection fits with the new parameter. It might be best to redo this code later now that I see more of the big picture.

ChristopherDavisUCI and others added 30 commits October 23, 2021 14:48
The way the schema is written for TopLevelRepeatSpec gives the current code trouble.  We patch it in an ad hoc way, and should later find a more sustainable method.
Instead of patching the downloaded schema, we change the definition of RepeatChart.  Currently the test_chart_from_dict test from altair/vegalite/v4/tests/test_api.py is failing on base.repeat(["c", "d"]).
@mattijn
Copy link
Contributor

mattijn commented Nov 9, 2021

This is silly but I don't understand why it lists so many commits. Most of those old commits were already merged. Is there an easy way to fix that? If I were left to my own, I would just create a new branch and move over the changed files (there are only a few I made changes in).

I think the proper method is to fetch master first, rebase your branch and then do a pull request, but I think I would do what you wrote.

@ChristopherDavisUCI
Copy link
Contributor Author

Thank you @mattijn I won't tell you which of the methods I ended up using :)
I made a new pull request #2517 that now only shows two commits, so that's better. I will close this one.

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.

None yet

2 participants