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

gr.ScatterPlot component #2764

Merged
merged 34 commits into from
Dec 9, 2022
Merged

gr.ScatterPlot component #2764

merged 34 commits into from
Dec 9, 2022

Conversation

freddyaboulton
Copy link
Collaborator

@freddyaboulton freddyaboulton commented Dec 5, 2022

Description

Overview

Implements a gr.ScatterPlot component. My plan is to have a separate component for each type of plot but they all inherit for gr.Plot. They should only differ from Plot in their init signatures and postprocess methods. This saves us from creating a different ui component for each plot subtype.

Proposed API

1. Create a static scatter plot

gr.ScatterPlot(value=cars, x="Horsepower", y="Miles_per_Gallon",
                        color="Origin", tooltip="Name", title="Car Data", y_title="Miles per Gallon")

2. Update an existing plot

return gr.ScatterPlot.update(
    value=cars,
    x="Horsepower",
    y="Miles_per_Gallon",
    color="Origin",
    tooltip="Name",
    title="Car Data",
    y_title="Miles per Gallon",
    color_legend_title="Origin of Car",
    caption="MPG vs Horsepower of various cars"
)

The value parameter must be passed in order to be able to update the plot.

Additional features added:

  • .style(container=False) added to Plot component
  • A caption can be added below gr.Scatterplot (and future plot types)

Open questions

1. Position of caption relative to the plot

The caption and plot are technically centered but it may look off-centered if there is a legend on the right of the plot

image

We can make it look more centered by placing all legends in the bottom of the plot. Wondering what people think and any other possible solutions.

image

2. Make labels + titles bold font or not

Using bold makes this info stand out but it looks different from the other fonts on the app even though they are technically the same font. For comparison

Bold (current):
image

No Bold:
image

3. General styling/look of the plots

Please send any feedback on the look of the plots. Tried my best to make sure they match the look of gradio but there could still be more to do. In particular:

  • Fonts
  • Text colors
  • Colors used in legends

To test

  1. Runpython demo/native_plots/run.py (also deployed to spaces)
  2. The following script
import gradio as gr
from vega_datasets import data


cars = data.cars()

io = gr.Interface(fn=lambda:cars,
                     inputs=None,
                     outputs=[gr.ScatterPlot(x="Horsepower", y="Miles_per_Gallon", color="Cylinders")])

with gr.Blocks() as demo:
    with gr.Row():
        with gr.Column():
            gr.ScatterPlot(value=cars, x="Horsepower", y="Miles_per_Gallon",
                    color="Origin", tooltip="Name", title="Car Data", y_title="Miles per Gallon")
        with gr.Column():
            gr.ScatterPlot(show_label=False,
                        value=cars,
                        x="Horsepower",
                        y="Miles_per_Gallon",
                        color="Origin",
                        tooltip="Name",
                        title="Car Data",
                        y_title="Miles per Gallon",
                        legend_title="Origin of Car").style(container=False)
        with gr.Column():
            gr.ScatterPlot(value=cars, x="Horsepower", y="Miles_per_Gallon",
                           tooltip="Name", title="Car Data").style(container=False)
    with gr.Row():
        io.render()

demo.launch()

image

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

The demo notebooks don't match the run.py files. Please run this command from the root of the repo and then commit the changes:

pip install nbformat && cd demo && python generate_notebooks.py

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

The demo notebooks don't match the run.py files. Please run this command from the root of the repo and then commit the changes:

pip install nbformat && cd demo && python generate_notebooks.py

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-2764-all-demos

@freddyaboulton freddyaboulton self-assigned this Dec 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

The demo notebooks don't match the run.py files. Please run this command from the root of the repo and then commit the changes:

pip install nbformat && cd demo && python generate_notebooks.py

@@ -192,6 +192,7 @@ function create_custom_element() {
root: ShadowRoot;
wrapper: HTMLDivElement;
_id: number;
theme: string;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes here are so that the Plot component has access to the current theme

export let theme: string;
export let caption: string;

function get_color(index: number) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I yanked this from Chart.svelte. Ideally this would be consolidated but it relies on the global colors prop so I wasn't sure what the best way to refactor was.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should produce the same colors for qualitative scales as the Chart.svelte (e.g. TimeSeries) component which is why I used it here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably want to update the color schemes for visualisations in the future with a few pre-baked options for users. Data-viz colour schemes have some specific requirements/ demands that fall outside of typical 'design' issues. We can address that (and the duplication) later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree! Filed #2780

const config = create_config(darkmode);
spec['config'] = config;
switch (value['chart'] || '') {
case "scatter":
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that we'll have a separate case for each plot type. I'd expect this code to change in the future though as we add more types.

@freddyaboulton freddyaboulton requested review from aliabid94, pngwn, abidlabs and aliabd and removed request for aliabid94 December 7, 2022 20:54
@freddyaboulton freddyaboulton changed the title [WIP] gr.ScatterPlot component gr.ScatterPlot component Dec 7, 2022
@@ -260,6 +262,7 @@ function create_custom_element() {
const _autoscroll = autoscroll === "true" ? true : false;

this.wrapper.style.minHeight = initial_height || "300px";
console.log(this.theme);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove.


$: if(value && value['type'] == "altair") {
spec = JSON.parse(value['plot'])
const config = create_config(darkmode);
spec['config'] = config;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
spec['config'] = config;
spec.config = config;

This is more idiomatic.

}
}

$: darkmode = theme == "dark"

$: if(value && value['type'] == "altair") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$: if(value && value['type'] == "altair") {
$: if(value && value.type == "altair") {


$: if(value && value['type'] == "altair") {
spec = JSON.parse(value['plot'])
const config = create_config(darkmode);
spec['config'] = config;
switch (value['chart'] || '') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
switch (value['chart'] || '') {
switch (value.chart || '') {

Comment on lines 40 to 46
if (spec['encoding']['color'] && spec['encoding']['color']['type'] == 'nominal') {
spec['encoding']['color']['scale']['range'] = spec['encoding']['color']['scale']['range'].map((e, i) => get_color(i));
}
else if (spec['encoding']['color'] && spec['encoding']['color']['type'] == 'quantitative') {
spec['encoding']['color']['scale']['range'] = ['#eff6ff', '#1e3a8a'];
spec['encoding']['color']['scale']['interpolate'] = "hsl";
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't suggest but use dot notation for property access: object.prop1.prop2.prop3 etc.

//@ts-nocheck
import Plotly from "plotly.js-dist-min";
import { Plot as PlotIcon } from "@gradio/icons";
import { colors as color_palette, ordered_colors } from "@gradio/theme";
import { get_next_color } from "@gradio/utils";
import { Vega } from "svelte-vega";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably want to dynamically load the SDK for the specific plot in the future but this is fine for now. Just mentioning it here. No changes required.

export let theme: string;
export let caption: string;

function get_color(index: number) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably want to update the color schemes for visualisations in the future with a few pre-baked options for users. Data-viz colour schemes have some specific requirements/ demands that fall outside of typical 'design' issues. We can address that (and the duplication) later.

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Not sure about the pending design questions but I think we can solve them in a follow-up, if needed.

CHANGELOG.md Outdated
@@ -2,6 +2,41 @@

## New Features:

### Scatter plot component

It is now possible to create a scatter plot without knowledge of a plotting library!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion, feel free to ignore

Suggested change
It is now possible to create a scatter plot without knowledge of a plotting library!
It is now possible to create a scatter plot natively in Gradio!

gradio/components.py Outdated Show resolved Hide resolved
@@ -294,7 +294,9 @@ def test_slider_random_value_config(self):
assert not any([dep["queue"] for dep in demo.config["dependencies"]])

def test_io_components_attach_load_events_when_value_is_fn(self, io_components):
io_components = [comp for comp in io_components if not (comp == gr.State)]
io_components = [
comp for comp in io_components if comp not in [gr.State, gr.ScatterPlot]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gr.ScatterPlot should also be able to take in a function for the value, no?

Copy link
Member

@abidlabs abidlabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Powerhouse of a PR @freddyaboulton! I only had a suggestion for the order of the parameters in the class, but otherwise LGTM. Tested and works great

@freddyaboulton
Copy link
Collaborator Author

Thank you all so much for the help and reviews!

@freddyaboulton freddyaboulton merged commit f60053d into main Dec 9, 2022
@freddyaboulton freddyaboulton deleted the scatter-plot branch December 9, 2022 15:14
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.

None yet

3 participants