-
Notifications
You must be signed in to change notification settings - Fork 606
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
feat: support autosize fit for faceted plots #6672
base: next
Are you sure you want to change the base?
Conversation
5191710
to
bac6abd
Compare
4369cd0
to
951f25b
Compare
Hi @drgould, do you plan to finish this pull request? |
Sorry, yeah I haven't had a chance to get back to this. My team has been hard at work implementing vega-lite into our product and this kinda fell on the back burner. We just launched it on Tues. so now were starting to pick back up some of this tech debt. The version we're currently using is a bit behind and we've been in need some of the new features so this will likely be prioritized very soon. |
Oh, that's awesome to hear. Congrats on launching the feature 🎉 . If you want, you can send a pull request to add it to https://vega.github.io/vega-lite/ecosystem.html. More generally, I would be happy to collaborate more closely on features and make sure to do a quick review/release turn around when you need a particular feature for your product. You have probably seen our roadmap and if there are bigger things you would like to tackle, we can add it there as well. |
Hi @domoritz @drgould ! Do you need any help to merge this ? I'd be glad to help on that in order to have this into vega-lite :) |
@wmarques yes, I won't have the cycles to implement this but I can review a pull request. Could you open a new pull request with this code and any improvements you want to make? |
@domoritz Sure, i'll check if this code works for us first, thanks for the quick answer ! |
Thank you for offering to help. I look forward to your contributions. |
@wmarques If all you need is a "responsive" grouped bar it's fairly easy to accomplish without faceting using a few transforms and a As for finishing up this PR, it's finally in my tasks for this sprint so I should be finishing it up very soon. |
@drgould Oh thanks alot I'll look into your example ! |
Just wanted to check in to see whether you need any help with getting this over the finish line. Please mark the pull request as ready for review when you want me to look at it. |
951f25b
to
4e71dc4
Compare
Ok I think I have this in a good enough place to open this up for review. I need a bit of guidance about any additional unit/spec/visual tests that might help as well as for the changes to the docs. Edit: looking into the failing tests. |
01b8651
to
5b7ba8d
Compare
The example generation depends on specifics of the OS so we usually let the ci do it (see contributing docs). If the ci doesn't do it for you, can you check your setup in your fork? I can help if there is some issue with it. |
ad7fcdc
to
5bd94d5
Compare
Yeah there definitely seems to be an issue with how the fork is set up. The really weird thing is it was working when I originally created this PR and didn't touch any of the workflows/etc. so ¯\_(ツ)_/¯ The only thing I see that's obviously wrong is the check workflow can't commit likely because the actions bot key is meant for this repo rather than the fork. But regardless the examples still aren't building correctly. |
Can you try setting a You can definitely build the schema locally, though (using |
I copied the pull request to our repo over at #7356 so we can build the examples. |
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.
This is great and pretty close to being done! I also just finished a deadline so I have more time to do faster reviews. If you can address the issues I found, I think we can merge this soon.
Here are a few problems I found.
Specs with row/column and autosize do not work.
{
"$schema": "https://vega.github.io/schema/vega-lite/v5.json",
"width": 500,
"height": 500,
"autosize": {
"type": "fit"
},
"data": {"url": "data/barley.json"},
"mark": "bar",
"encoding": {
"column": {"field": "year"},
"x": {"field": "yield", "type": "quantitative", "aggregate": "sum"},
"y": {"field": "variety", "type": "nominal"},
"color": {"field": "site", "type": "nominal"}
}
}
Note that this spec also generates a warning "Autosize "fit" only works for single views and layered views.", which is not correct anymore since we do support fit for faceted plots as well.
This should be a simple fix in the normalized where we should generate.
{
"$schema": "https://vega.github.io/schema/vega-lite/v5.json",
"autosize": {"type": "fit"},
"width": 500,
"height": 500,
"padding": 0,
"data": {"url": "data/barley.json"},
"facet": {"column": {"field": "year"}},
"spec": {
"mark": "bar",
"encoding": {
"x": {"field": "yield", "type": "quantitative", "aggregate": "sum"},
"y": {"field": "variety", "type": "nominal"},
"color": {"field": "site", "type": "nominal"}
}
}
}
Adding autosize to x in this chart with wrapped facet breaks it
{
"$schema": "https://vega.github.io/schema/vega-lite/v5.json",
"name": "trellis_barley",
"width": 500,
"autosize": {
"type": "fit-x"
},
"padding": 0,
"data": {"url": "data/barley.json"},
"columns": 2,
"facet": {
"field": "site",
"type": "ordinal",
"sort": {"op": "median", "field": "yield"}
},
"spec": {
"height": {"step": 12},
"mark": "point",
"encoding": {
"x": {
"aggregate": "median",
"field": "yield",
"type": "quantitative",
"scale": {"zero": false}
},
"y": {"field": "variety", "type": "ordinal", "sort": "-x"},
"color": {"field": "year", "type": "nominal"}
}
}
}
@drgould is there anything I can help with to get this pull request over the finish line? |
Sorry been working on some other things and haven't had a chance to get back to this. I'll hopefully have some time to make those changes soon. |
Hello. Doest the pyramidal distribution (ie: using hconcat) will be affected by this feature and became responsive ? Thank you. Code: Vega editor |
No, this is for faceted charts only. We already support single view and layered plots. |
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 so much for submitting the PR! It's not very common that people digged deep into the codebase to submit enhancement, so we really appreciate that. I think this would be an awesome addition for Vega-Lite.
To add to @domoritz's point, this PR currently only have one example, which might not cover different cases we want to support.
To make sure that we have tested (and have regression tests) for different cases, could you please include more examples, showing combinations of the following variations?
-
Width/Height -- currently in the only example provided, we see that width and height are only outside. However, it would be great to have examples that test when there is a width at the facet level, but the height is based on the unit spec's height.
-
Facet -- In the only example provided, the example is a facet column. And you include facet row, facet row x column, and wrapped facet?
If some of these cases do not work, please instead add warning messages by using log.warn() from src/log
and add new log messages to src/log/messages
.
@@ -98,12 +98,16 @@ Below is an example of a bar chart that fits exactly into 300px width and the de | |||
|
|||
## Width and Height of Multi-View Displays | |||
|
|||
The width and height of multi-view displays including [concatenated](concat.html), [faceted](facet.html), and [repeated](repeat.html) are determined based on the size of the composed unit and layered views. To adjust the size of multi-view displays, you can set the `width` and `height` properties of the inner unit and layered views. | |||
The width and height of multi-view displays including [concatenated](concat.html), [faceted](facet.html), and [repeated](repeat.html) are generally determined based on the size of the composed unit and layered views. To adjust the size of multi-view displays, you can set the `width` and `height` properties of the inner unit and layered views. |
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.
generally
"by default"?
@drgould Is there anything we can help with to get this pull request over the finish line? |
Our priorities shifted quite significantly a couple months ago and I haven't had any time to get back to this. I was hoping I would be able to carve out some time to finish this up but realistically I won't be able to do any work on this for at least 3 months. At this point it might be better if someone else takes this over. |
is this likely to get released soon? its exactly what i need |
Since it's not merged, no. Please help us push it over the finish line if you need it. |
Interested in this one as well. |
Would you be able to summarize what is needed for this to be completed? I may have some time to look into it! |
That's awesome. It's been a while so I can't summarize it but you should find everything in the comments in this pull request. Let us know if you have specific questions. |
Based on discussion in #5219 and stealing some code from #5225, this adds support for specifying the outer plot dimensions and sizing the inner facets automatically which subsequently allows unlocking
autosize: fit(-x/-y)
for faceted plots.Note: this is only for specs which explicitly define the facet operator at the top level.