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

Revert legend explicit positioning since it breaks VS Code #115

Closed
shcheklein opened this issue Feb 5, 2023 · 7 comments
Closed

Revert legend explicit positioning since it breaks VS Code #115

shcheklein opened this issue Feb 5, 2023 · 7 comments
Assignees
Labels
A: vega Area: Vega plots enhancement New feature or request priority-p1 Important to address soon. Aka current active backlog.

Comments

@shcheklein
Copy link
Member

shcheklein commented Feb 5, 2023

I suggest we revert #113. Plots have become unmanageable and I can't find a quick fix for this:

Screenshot 2023-02-04 at 4 43 26 PM

Screenshot 2023-02-04 at 3 50 07 PM

If there are more rev selected plot becomes smaller and smaller.

Here is the Vega config with the VS Code customizations (including an explicit instruction to disable the legend, but it's not working in this case):

```json { "$schema": "https://vega.github.io/schema/vega-lite/v5.json", "data": { "values": [ { "step": 0, "accuracy": "0.41710779070854187", "dvc_data_version_info": { "filename": "evaluation/plots/metrics/train/accuracy.tsv", "field": "accuracy" }, "filename": "evaluation/plots/metrics/train/accuracy.tsv", "field": "accuracy", "rev": "workspace" }, { "step": 1, "accuracy": "0.46896055340766907", "dvc_data_version_info": { "filename": "evaluation/plots/metrics/train/accuracy.tsv", "field": "accuracy" }, "filename": "evaluation/plots/metrics/train/accuracy.tsv", "field": "accuracy", "rev": "workspace" }, { "step": 0, "accuracy": "0.47241726517677307", "dvc_data_version_info": { "filename": "evaluation/plots/metrics/eval/accuracy.tsv", "field": "accuracy" }, "filename": "evaluation/plots/metrics/eval/accuracy.tsv", "field": "accuracy", "rev": "workspace" }, { "step": 1, "accuracy": "0.508525550365448", "dvc_data_version_info": { "filename": "evaluation/plots/metrics/eval/accuracy.tsv", "field": "accuracy" }, "filename": "evaluation/plots/metrics/eval/accuracy.tsv", "field": "accuracy", "rev": "workspace" } ] }, "title": "dvc.yaml::accuracy", "width": 300, "height": 300, "params": [ { "name": "smooth", "value": 0.001, "bind": { "input": "range", "min": 0.001, "max": 1, "step": 0.001 } } ], "layer": [ { "mark": "line", "encoding": { "x": { "field": "step", "type": "quantitative", "title": "step" }, "y": { "field": "accuracy", "type": "quantitative", "title": "accuracy", "scale": { "zero": false } }, "color": { "field": "rev", "type": "nominal", "legend": { "orient": "top", "direction": "vertical" } } }, "transform": [ { "loess": "accuracy", "on": "step", "groupby": [ "rev", "filename", "field", "filename::field" ], "bandwidth": { "signal": "smooth" } } ] }, { "mark": { "type": "point", "tooltip": { "content": "data" } }, "encoding": { "x": { "field": "step", "type": "quantitative", "title": "step" }, "y": { "field": "accuracy", "type": "quantitative", "title": "accuracy", "scale": { "zero": false } }, "color": { "field": "rev", "type": "nominal" } } } ], "encoding": { "color": { "legend": { "disable": true }, "scale": { "domain": [ "workspace" ], "range": [ "#945dd6" ] } }, "strokeDash": { "field": "filename", "scale": { "domain": [ "evaluation/plots/metrics/eval/accuracy.tsv", "evaluation/plots/metrics/train/accuracy.tsv" ], "range": [ [ 1, 0 ], [ 8, 8 ] ] }, "legend": { "disable": true } } } } ```
@shcheklein shcheklein added enhancement New feature or request A: vega Area: Vega plots labels Feb 5, 2023
@shcheklein shcheklein added the priority-p1 Important to address soon. Aka current active backlog. label Feb 5, 2023
@mattseddon
Copy link
Member

mattseddon commented Feb 6, 2023

Alternative from iterative/vscode-dvc#3226 => VS Code walks the templates and inserts the correct information into the templates at the correct place instead of applying overrides at the bottom of the template's encoding.

@shcheklein
Copy link
Member Author

Agreed, I think we can traverse and drop the legend objects as we do with titles. It should be safe I think. Let me try do this first.

@dberenbaum dberenbaum reopened this Feb 6, 2023
@dberenbaum
Copy link
Contributor

Since the main rationale of #113 was to improve the UI in Studio and VS Code, should we still consider reverting the change? Is it likely to always be important to have custom legend handling in those products, and does that change make it harder?

@mattseddon
Copy link
Member

@dberenbaum we can remove legends programmatically (see iterative/vscode-dvc#3228) so we don't need to revert the change.

However, take a look at this PR: iterative/vscode-dvc#3229

I tried moving the legend to the top for the zoomed plots behaviour. When the User selects the max number of revisions (7) this gets pretty unwieldy. I think we will be keeping our legends on the right for now.

How about we chat about this one in our next cross-team meeting.

@dberenbaum
Copy link
Contributor

I'd still vote to revert for consistency across products. If the legend is on the right in VS Code, let's do it everywhere unless we have a good reason. Removing the legends programmatically also just feels like adding more places things can break, so why not revert and save the trouble?

@mattseddon
Copy link
Member

Up to you.

I think VS Code will still need to programmatically remove legends. This change has demonstrated that custom templates are definitely going to break the VS Code UI.

@shcheklein
Copy link
Member Author

Since we don't prioritize custom templates for now, I think we should be good. @dberenbaum your call on this. I can close the PR then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: vega Area: Vega plots enhancement New feature or request priority-p1 Important to address soon. Aka current active backlog.
Projects
None yet
Development

No branches or pull requests

4 participants