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

Change default chart width from 400 to 300 #2785

Merged
merged 5 commits into from
Jan 8, 2023

Conversation

binste
Copy link
Contributor

@binste binste commented Dec 30, 2022

Closes #2581

@mattijn
Copy link
Contributor

mattijn commented Dec 30, 2022

I think its good to get the approval of @joelostblom here.

Just a note: I find myself very often removing the following definition from the generated Vega-lite specification by Altair:

"config": {"view": {"continuousWidth": 400, "continuousHeight": 300}},

@mattijn mattijn requested a review from joelostblom December 30, 2022 21:35
@joelostblom
Copy link
Contributor

Looks good to me overall, but does it make sense to change this in the v3 and v4 versions as well or should we consider them as "frozen" unless we decide to make another 3.x or 4.x point release?

@binste
Copy link
Contributor Author

binste commented Jan 3, 2023

I'd find it a bit more intuitive as a user if we also do it for v3 and v4 as other features are usually also applied to those. these folders are not a snapshot of Altair 3 and 4 but instead of the corresponding vega-lite schemas.

@joelostblom
Copy link
Contributor

Hmm maybe I am thinking about this wrong, but wouldn't we in general want to avoid adding new altair features to the old schemas because we might add something in altair that would rely on new Vega-Lite functionality that would not be available in the old schemas and therefore potentially cause an error?

At least this was the thinking I had when I remove the re-generation of the old schemas as part of https://github.com/altair-viz/altair/pull/2795/files#:

image

The other side of this I guess is that unless we decide to make a new release of 3.x or 4.x, users would not be able to benefit from any updates we make, so what is the benefit of adding these updates also to the old schemas?

@binste
Copy link
Contributor Author

binste commented Jan 4, 2023

Ah I wasn't aware of that change, thanks for the screenshot!

Honestly, I have never seen a use case so far where the old schemas were used and so I don't know what users are expecting from those folders. I can see your point about new vega-lite functionality being introduced. I'm reverting again the changes to v3 and v4 but let me know if you want to keep it after all.

@mattijn
Copy link
Contributor

mattijn commented Jan 4, 2023

While I don't use it as such, sometimes you see:

import altair.vegalite.v3 as alt  # or .v4 as alt

This approach is also documented https://altair-viz.github.io/user_guide/importing.html#importing-vega-vega-lite-versions (I noticed the page has been updated already in the main repository).

@binste
Copy link
Contributor Author

binste commented Jan 4, 2023

Indeed. When would you actually use this over simply installing an older version of Altair? Maybe if you somehow still need to produce based on an old VL version but a new version of Altair contains a useful bugfix? But what if that bugfix is now no longer applied to v3 and v4 based on the changes to generate_schema_wrapper.py?

Furthermore, v3 and v4 are not truly frozen as they import both from altair.utils, for example https://github.com/altair-viz/altair/blob/b1774e673423af87ff09a49567592c52479be349/altair/vegalite/v4/schema/core.py#L4

Maybe this is too bold but in case

  • this functionality is indeed not used that often (not sure how to find out apart from googling a bit)
  • and we don't have a good understanding of which changes should go into previous version folders and which should not

what about removing previous version folders completely? It would simplify the maintenance of Altair.

@mattijn
Copy link
Contributor

mattijn commented Jan 4, 2023

Good points @binste! Thanks for raising and not at all too bold.

The altair repository has naturally evolved with both vega and vega-lite-schemas. It is attempted to keep up with these changes by writing python code that can handle any version of the schemas, including vega-lite v1, v2, v3, v4 and now v5.

From a technical perspective it's hard, but it is also a great achievement to make this all work. From a maintenance perspective it's just hard.

I'm not sure exactly what was the rationale from @joelostblom to introduce the breaking changes by not backporting anything here https://github.com/altair-viz/altair/blob/master/tools/generate_schema_wrapper.py#L32

From a user perspective you could gradually adopt to new features and functionality. Maybe one person write altair code as a python developer/data scientist where the generated vega-lite is feed into a web-application maintained by a front-ender which is still on a certain version of Vega/Vega-lite because it has to maintain new and also legacy code.

Personally I would favour that the new release of Altair is able to generate vega-lite specifications that can match both version 4 and version 5 of Vega-lite (not against deprecating the Vega-lite schema v3), especially with these changes from selection to parameters.

If that means that one can create vegalite vl4-style-json using this method-syntax with the new release of altair, that would be fine isn't it? Or I'm missing something here?

@joelostblom
Copy link
Contributor

I will start by saying that I don't feel too strongly about this so if both of you are on board with keeping v4 (and maybe just removing the v3 folder), then I'm on board with that and we can add it back here https://github.com/altair-viz/altair/blob/master/tools/generate_schema_wrapper.py#L32.

From my end, I still don't quite see the benefit of keeping any of the old versions around. It does seem like the best way to keep compatibility with an older version of vega-lite is to install an older version of Altair (or maybe specify a specific VL version to vl-convert?).

I also quite don't understand what keeping older versions around means for adding new functionality. Should we always add it it in multiple places? Take #2732 for example, should that change have been added to v4/api.py as well?

I'm not sure exactly what was the rationale from @joelostblom to introduce the breaking changes by not backporting anything here

I think my initial thinking when I did that was that we didn't want users using Altair 4.x to get unwanted new features, but I realize that this will not happen unless they actually install Altair 5.x. Although I still don't quite understand the benefit of keeping the old versions around I'm ok with reverting this change if you think it is too drastic.

@binste
Copy link
Contributor Author

binste commented Jan 6, 2023

Great inputs! My understanding now is that we have two viable options

  1. Keep v4 and add it back here https://github.com/altair-viz/altair/blob/master/tools/generate_schema_wrapper.py#L32. This allows users to upgrade to Altair 5 to use new features such as method-based attribute setting but they can still produce v4 vega-lite specs in case they need to do this for e.g. some web application as @mattijn mentioned. Personally, in such a case I would just create those charts for the web application with Altair 4 in a separate environment.
  2. If we do not apply new features to v4, I also don't see a need to maintain multiple versions as then someone could just install Altair 4.

I slightly favour option 2, as I think also @joelostblom does, but maybe I don't see all the use cases which speak for option 1.

Personally I would favour that the new release of Altair is able to generate vega-lite specifications that can match both version 4 and version 5 of Vega-lite (not against deprecating the Vega-lite schema v3), especially with these changes from selection to parameters.

@mattijn Could you elaborate on why the changes from selection to parameters would speak for keeping version 4 around? Is it so that users can install Altair 5 but still use the code for their interactive v4 charts based on selections by importing the v4 version from Altair 5? Wouldn't it then be simpler for them to never upgrade in the first place?

@mattijn
Copy link
Contributor

mattijn commented Jan 6, 2023

It always has been presented as such,

Altair Version Vega-Lite Version
v1 v1
v2 v2
v3 v3
v4 v4

So for me, to follow this logic is fine:

Altair Version Vega-Lite Version
v5 v5

But the real story was and still is a bit different. There was also always some backward support for the previous VL version and support for Vega version(s). See the following table:

Altair Version Vega-Lite Version Vega Version
v1 v1.0.12 N/A
v1.2.0 v1.2.1 N/A
v1.2.1 v1.2.1 N/A
v2.0.0rc1 v1.3.1, v2.3.0 v2.6.5, v3.2.1
v2.0.0rc2 v1.3.1, v2.3.0 v2.6.5, v3.2.1
v2.0.0 v1.3.1, v2.3.0 v2.6.5, v3.2.1
v2.0.1 v1.3.1, v2.4.1 v2.6.5, v3.3.1
v2.1.0 v1.3.1, v2.4.3 v2.6.5, v3.3.1
v2.2.0 v1.3.1, v2.6.0 v2.6.5, v3.3.1, v4.0.0
v2.2.1 v1.3.1, v2.6.0 v2.6.5, v3.3.1, v4.0.0
v2.2.2 v1.3.1, v2.6.0 v2.6.5, v3.3.1, v4.0.0
v2.2.3 v1.3.1, v2.6.0 v2.6.5, v3.3.1, v4.0.0
v2.3.1 v1.3.1, v2.6.0 v2.6.5, v3.3.1, v4.0.0
v2.4.1 v1.3.1, v2.6.0 v2.6.5, v3.3.1, v4.0.0
v3.0.0rc1 v2.6.0, v3.0.2 v3.3.1, v4.4.0, v5.3.1
v3.0.0 v2.6.0, v3.2.1 v3.3.1, v4.4.0, v5.3.5
v3.0.1 v2.6.0, v3.2.1 v3.3.1, v4.4.0, v5.3.5
v3.1.0 v2.6.0, v3.3.0 v4.4.0, v5.4.0
v3.2.0 v2.6.0, v3.4.0 v4.4.0, v5.4.0
v3.3.0 v2.6.0, v3.4.0 v4.4.0, v5.4.0
v4.0.0 v3.4.0, v4.0.0 v5.9.0
v4.0.1 v3.4.0, v4.0.2 v5.9.0
v4.1.0 v3.4.0, v4.8.1 v5.10.0
v4.2.0rc1 v3.4.0, v4.17.0 v5.10.0
v4.2.0 v3.4.0, v4.17.0 v5.10.0

As you can see, most of the time there were multiple versions of Vega-Lite and Vega supported.
Since Altair 4 the number of Vega support has been reduced to a single supported version.

Maybe the time has come to do the same for Vega-Lite.

But be clear why, because I'm not sure if the argument of breaking changes hold. It will reduce maintenance that is for sure.

Maybe the time has come as well to drop support for Vega completely? Since you can use vl-convert to render Vega as svg/png. I'm not sure if vl-convert validates the specification too.

I meant the following with selection and parameters: one can write the same Altair specification, where it creates different type of Vega-lite specification, just by defining which vegalite version you like to use.

from vega_datasets import data
cars = data.cars.url

import altair.vegalite.v5 as alt

alt.Chart(cars).mark_point().encode(
    x=alt.X('Horsepower:Q'),
    y=alt.Y('Miles_per_Gallon:Q')
).interactive().to_dict()
{'config': {'view': {'continuousWidth': 400, 'continuousHeight': 300}},
 'data': {'url': 'https://cdn.jsdelivr.net/npm/vega-datasets@v1.29.0/data/cars.json'},
 'mark': 'point',
 'encoding': {'x': {'field': 'Horsepower', 'type': 'quantitative'},
  'y': {'field': 'Miles_per_Gallon', 'type': 'quantitative'}},
 'params': [{'name': 'param_1',
   'select': {'type': 'interval', 'encodings': ['x', 'y']},
   'bind': 'scales'}],
 '$schema': 'https://vega.github.io/schema/vega-lite/v5.2.0.json'}
import altair.vegalite.v4 as alt

alt.Chart(cars).mark_point().encode(
    x=alt.X('Horsepower:Q'),
    y=alt.Y('Miles_per_Gallon:Q')
).interactive().to_dict()
{'config': {'view': {'continuousWidth': 400, 'continuousHeight': 300}},
 'data': {'url': 'https://cdn.jsdelivr.net/npm/vega-datasets@v1.29.0/data/cars.json'},
 'mark': 'point',
 'encoding': {'x': {'field': 'Horsepower', 'type': 'quantitative'},
  'y': {'field': 'Miles_per_Gallon', 'type': 'quantitative'}},
 'selection': {'selector001': {'type': 'interval',
   'bind': 'scales',
   'encodings': ['x', 'y']}},
 '$schema': 'https://vega.github.io/schema/vega-lite/v4.17.0.json'}

Here can be seen that the v5 now uses params and v4 still uses selection in the top-level for the interactivity definition.

This might be unwanted in some cases (maybe a follow-up pipeline expect selection in the spec, or maybe the javascript version of vegalite is still pinned to 4.17 for all your company web-apps) and then it is/was nice that you have/had a choice within a single package.

I think therefor people also use something as import altair.vegalite.v4 as alt, to make sure they are generating vega-lite specification of a certain version and not whatever import altair as alt will generate. Especially if Altair will be installed in an image/container part of an automated process.

(btw, Vega-lite v5 has still support for selection, so one can also use a Vega-lite v4 generated specification in Vega-lite 5, but no, v4 has no future support for params)

Again, I'm OK with including support for a single VL version (and I think even with no version of Vega).

@mattijn
Copy link
Contributor

mattijn commented Jan 6, 2023

Its not really 'again', but more like, 'I could live with'.

@binste
Copy link
Contributor Author

binste commented Jan 7, 2023

The table is very helpful! Did you create it based on the repo history/CHANGELOG.md or is this available somewhere?

My goal with the comments below is not to convince anyone, I think the two of you have a much better understanding of how the package is currently used then I do. I'll just try to provide some more food for thought in case it helps you in making a decision:

  • I still see the addition of new features in Altair which are independent of the VL version, such as support in Chart.save for vl-convert-python, as a good argument to keep v4 folder around. This is something that users couldn't profit from if they'd have to stick with Altair 4 to get VL 4 specs. Although in this example they could just call the function vlc.vegalite_to_png(chart.to_json())
  • Regarding the topic of breaking changes and maintenance, my understanding of the code generation process is that currently it's unclear how changes in a new VL version (e.g. 5) could be applied in case they would lead to breaking changes in an older version (e.g. 4). You can stop the regeneration of the old code (as is currently the case) but this will then happen for all features, i.e. there is no way to selectively apply certain changes to older VL folders. But I can't think of a good example here so maybe this is more a theoretical worry and not relevant.
  • A big part of the test suite in Altair are the examples which are being executed only for the newest VL version. So it can happen that I for example make a change in altair.utils which works for v5 but creates a bug in v4 (where it is imported as mentioned further above) which might not show up on any tests.
  • Thank you for elaborating on the selection and params example. I see that this gives users an additional way to "pin" the vega-lite version. However, I actually think this should be discouraged and pinning should happen via the Altair package version. If, as you mentioned, Altair is installed as part of an automated process and then produces charts for some web application which expects a certain vega-lite version, installing the newest Altair version and importing import altair.vegalite.v4 as alt would break the whole pipeline once Altair 6 comes out in case the v4 folder is removed. Following the Zen of Python "There should be one-- and preferably only one --obvious way to do it." ;) Same of course for all other Python packages which are used in such a pipeline.
  • Good point about Vega and vl-convert! I just checked what happens when I call vlc.vegalite_to_vega on a VegaLite spec which as an invalid encoding channel. The conversion worked but it produced a warning, so I think it does not do validation. The VegaLite JS package probably produces this warning itself:
import vl_convert as vlc
vl_spec = r"""
{
  "$schema": "https://vega.github.io/schema/vega-lite/v5.json",
  "data": {"url": "https://raw.githubusercontent.com/vega/vega-datasets/next/data/movies.json"},
  "mark": "circle",
  "encoding": {
    "x": {
      "bin": {"maxbins": 10},
      "field": "IMDB Rating"
    },
    "y": {
      "bin": {"maxbins": 10},
      "field": "Rotten Tomatoes Rating"
    },
    "size": {"aggregate": "count"},
    "unknown_channel": {"maxbins": 10}
  }
}
"""
vg_spec = vlc.vegalite_to_vega(vl_spec)
WARN unknown_channel-encoding is dropped as unknown_channel is not a valid encoding channel.
  • Regarding this topic, Jake mentioned in this comment to be thinking about removing Vega wrappers completely and removing v3 folder for VL.

@mattijn
Copy link
Contributor

mattijn commented Jan 7, 2023

The table is very helpful! Did you create it based on the repo history/CHANGELOG.md or is this available somewhere?

From the repo history. I used the tags of each released version to check the generate schema wrapper. Now we have a nice overview, useful for discussions. I opened this issue to continue this discussion further #2817 or start a PR against.

@joelostblom
Copy link
Contributor

I am going to merge this in. There are several PRs that have been adding new functionality only to VL 5, so if we want to support VL4 we will need to go through the recent PRs (including this one) and add the corresponding functionality to the v4 folder.

Thanks for all the great discussion of which versions to support in Altair 5, I think we can continue that in #2817.

@joelostblom joelostblom merged commit 178e9d7 into vega:master Jan 8, 2023
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.

Equal default chart dimensions
3 participants