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

Sample417 #2509

Closed
wants to merge 4 commits into from
Closed

Sample417 #2509

wants to merge 4 commits into from

Conversation

ChristopherDavisUCI
Copy link
Contributor

@ChristopherDavisUCI ChristopherDavisUCI commented Oct 24, 2021

Hi, based on https://github.com/mattijn/altair/tree/vl_v5 I tried to make a version that worked with Vega-Lite schema 4.17.0. I did very little other than replace some of the old code which assumed everything was either a "field" or "value", to allow for the possibility of a "datum". Much of the 'mixins.py' and the high-level Altair API are quite mysterious to me, so I didn't do anything intelligent there.

I assume there are some gaps in what I did, but I'm happy to keep working on it if I receive some pointers on what needs to be improved. Thanks for looking at it and I hope some of it might be helpful.

Here are two examples of the update. The pie chart example is mostly copied from mattijn's comment. So a few new things work, although I see from the GitHub checks that other things became broken. (I added a comment below.)

import altair as alt
from vega_datasets import data

source = data.stocks()

chart1 = alt.Chart(source).mark_line().encode(
    x = alt.X("date"),
    y = alt.Y("price"),
    color = "symbol"
)

chart2 = alt.Chart(source).mark_rule().encode(
    y = alt.YDatum(350)
)

chart1+chart2

rule2

import pandas as pd
import altair as alt

df = pd.DataFrame({'category': [1, 2, 3, 4, 5, 6], 'value': [4, 6, 10, 3, 7, 8]})

alt.Chart(df).mark_arc(innerRadius=40).encode(
    theta='value:Q',
    color='category:N',
)

arc

@ChristopherDavisUCI
Copy link
Contributor Author

(I'm happy to move these comments and questions somewhere else like one of the GitHub Issues or the Slack channel, if that's a more appropriate place. I'm also happy to close this Pull Request if that's recommended.)

I tried following the Testing your Changes instructions but all I received after running make test was:

make: *** [test] Error 1

The error messages from the GitHub checks here are more helpful.

Here is one minimal example of something that seems to go wrong. alt.OpacityValue(1).to_json() works fine in Altair, but in this development version, this code produces

SchemaValidationError: Invalid specification
altair.vegalite.v4.schema.channels.OpacityValue->0, validating 'type'
is not of type 'array'

On the other hand, alt.OpacityValue(1).to_json(validate=False) works fine in the development version.

To find the correct base class for OpacityValue (or anything else), the program traces through references in the vega-lite schema starting at FacetedEncoding->properties->opacity, and continuing until it reaches a definition that contains value among its properties. (Similarly for Opacity, tracing until it finds field, and for OpacityDatum, tracing until it finds datum.)

The way the code is written, only one base class for OpacityValue will ever be found. Does that seem to be a mistake? I could imagine finding multiple opacity base classes that match this condition of having value among their properties. Or I could also imagine using a definition higher up the chain of references.

Thanks for any suggestions!

@jakevdp
Copy link
Collaborator

jakevdp commented Oct 25, 2021

Yes, there is a fundamental issue with Altair's design that makes it difficult to suppot both Value and Datum encodings. The machinery was written for schemas that only have Value, and will have to be rewritten in order to accomodate Datum specifications if we want to upgrade beyond vega-lite 4.8.

@ChristopherDavisUCI
Copy link
Contributor Author

Thank you for the comments @jakevdp

I'll close this pull request.

Do you think the "parameters" which were introduced in Vega-Lite 5 will introduce a whole different set of issues? I haven't read about parameters at all and don't have a sense for whether they should be considered at the same time as datum.

@jakevdp
Copy link
Collaborator

jakevdp commented Oct 25, 2021

Yeah, I imagine the shift from selections to parameters will take some thinking, especially if we want to maintain any sort of backward compatibility with the API of previous versions.

I've really wanted to address all this and push for the upgrade, I've just not been at a place in my life where I can take on that project.

@ChristopherDavisUCI
Copy link
Contributor Author

Thanks again @jakevdp for the feedback.

I feel like I have a better sense for the scope of the project to upgrade.

I will maybe post a little later in this GitHub issue, to ask the general community a few questions. But if that's not an appropriate place or if there's somewhere better, I'm very open to that advice.

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