-
Notifications
You must be signed in to change notification settings - Fork 794
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
Update Vega-Lite to 5.14.1. Add transform_extent #3148
Conversation
Thanks for working on this @binste! Yes, I'll get a vl-convert update out shortly |
Vega-Lite 5.14.1 added to Update: I restarted the tests to see if that fixes the issue |
I don't know if it actually introduces any challenges, but the new From https://vega.github.io/vega-lite/docs/extent.html, it looks like the param is provided as just a string, so I guess that should work fine in Altair as well. It doesn't need to be this PR, but we should make a note that we need to add a new extent transform section to the docs, and it would be good to port over the example from https://vega.github.io/vega-lite/docs/extent.html. |
Thanks for making a new release of vl-convert and the feedback! Good point about I'll add it in this PR in the coming days
|
Implemented |
doc/user_guide/transform/index.rst
Outdated
@@ -96,6 +97,7 @@ Transform Supported | |||
:ref:`user-guide-bin-transform` ✔ | |||
:ref:`user-guide-calculate-transform` ✔ | |||
:ref:`user-guide-density-transform` | |||
:ref:`user-guide-extent-transform` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonmmease Does VegaFusion support the extent transform? The .transformed_data
method worked on the chart example I built in the doc page but as I use the parameter value as single values it does not return it anyway so not sure if that's a good test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, VegaFusion supports extent, so I think we're good there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'll add the tick in the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pulling all of this together, and adding the new docs for extent. lgtm!
Thanks for the review! :) |
So far there were no changes to Altair necessary apart from rerunning the automatic code generation. For the tests to pass, we need a new version of
vl-convert
with support for VL 5.14.1. @jonmmease Would that work for you? I think even if a new VL version comes out in the next 1-2 weeks, although I'm not aware of one, we could still stick to VL 5.14.1 for Altair 5.1.See #3145 (comment) for the related release discussion.