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

API Option #34

Closed
light-and-salt opened this issue Apr 12, 2016 · 22 comments
Closed

API Option #34

light-and-salt opened this issue Apr 12, 2016 · 22 comments
Assignees

Comments

@light-and-salt
Copy link
Contributor

Generally we want the user to write a field name without any underscore prefixes in options.fields and options.fieldConfigs. Then the tooltip plugin supplements the field names with underscore prefixes derived from vlSpec. We want to supplement the user-provided field names in order to match the field names returned by item.datum.

For aggregated fields, user should provide field name (e.g. 'yield'). Then tooltip supplements the field name with the aggregate operation (e.g. 'mean_yield'). This should match the field names in item.datum.

One exception is the aggregate operation count, where the field name is '*'. In this case the supplemented field name should be 'count' instead of 'count_*'

For fields with timeUnit, user should provide the field name (e.g. 'date'). Tooltip supplements the field name with the timeUnit (e.g. 'month_date').

For binned fields, user should provide field name (e.g. 'Acceleration'). Tooltip supplements the field name with 'bin_' (e.g. 'bin_Acceleration'). In runtime the tooltip plugin calculates the range of the bin field (issue #29).

If user don't provide options.fields, tooltip supplements options.fields with all fieldDefs in the vlSpec, using the underscore prefixes described above.

@kanitw
Copy link
Member

kanitw commented Apr 12, 2016

This is a very good point. One problem here is what if a particular field name has two functions? It's unclear how to resolve this.

Another possibly better alternative is to make the API a list of array instead of dictionary as I suggested yesterday. (And since it's an array, we can have field, bin, aggregate, timeUnit just like fieldDef.

@light-and-salt light-and-salt self-assigned this Apr 12, 2016
@light-and-salt
Copy link
Contributor Author

I see your point. I think I like the array approach better too as I don't have to overwrite user-provided field names :)

So with the array approach, the options will look like this

var options = 
{
    // user provides these
    fields: ['field1', 'field2', 'field3'],
    fieldConfigs: // user provides, vlSpec supplements
    {   
        'field1': 
        { 
            title: 'title1',
            type: 'number' | 'time' | undefined, 
            format: string-specifier
        }
    }
    colorTheme: 'light' | 'dark',

    // vlSpec supplements these
    bin: ['field1'],
    timeUnit: {'field2': 'yearmonth'},
    aggregate: {'field3': ['sum', 'mean']} 
};

When user provides a field name that doesn't match field names in item.datum, we consult bin, timeUnit and aggregate for resolution.

When user doesn't provide options.fields and we show all fields by default, we also consult bin, timeUnit and aggregate so that we can hide the underscore prefixes even when no options is provided.

@kanitw
Copy link
Member

kanitw commented Apr 13, 2016

This looks very complicated and it's not really an array?

Also,

When user provides a field name that doesn't match field names in item.datum, we consult bin, timeUnit and aggregate for resolution.

What if we want a field with multiple aggregation functions?

I really wonder if we really need to decouple field list from config?

This seems simpler, with additional showUnspecifiedFields, we don't really need to decouple them?

var options = 
{
    // user provides these
    fields: [
      {
         field: 'field1', 
         aggregate: ...,    // can be explicitly specified, but can be automatically inferred
         timeUnit: ...,       // can be explicitly specified, but can be automatically inferred
         bin: true | false   // can be explicitly specified, but can be automatically inferred
         title: ...,              // default = <func>(<field-name>)
         formatType: ..., // don't name this "type" it's confusing.  
         format: ...
      },
      ...
    }],
    showUnspecifiedFields: true | false // true by default?

    colorTheme: 'light' | 'dark'
};

@light-and-salt
Copy link
Contributor Author

If we don't decouple fields and fieldConfigs we still have the "to specify a format, you have to specify the field and all other fields you want to show" problem. To display a custom list of fields showUnspecifiedFields will be set to false. Then if I want to set the format of field1 while also showing field2 and field3 I will have to put field1, field2, and field3 in options.fields. This means the user has to type a bit more.

I think decoupling also works well with vlSpec supplementing formats. Imagine if vlSpec supplemented format for field1, then field1 is in the list of options.fields and will show up in the tooltip. This might not be the user's intention though.

@kanitw
Copy link
Member

kanitw commented Apr 13, 2016

we still have the "to specify a format, you have to specify the field and all other fields you want to show" problem

Well, showUnspecifiedFields solves this problem.

Then if I want to set the format of field1 while also showing field2 and field3 I will have to put field1, field2, and field3 in options.fields. This means the user has to type a bit more.

If you really worry about typing, we could support a shorthand like

fields: [{field: "field1", ...},  "field2", "field3"] 

Even without shorthand, the config proposal can be verbose and repeat field names multiple times, plus it is unclear how to deal with repeated fields with different functions as mentioned earlier.

Imagine if vlSpec supplemented format for field1, then field1 is in the list of options.fields and will show up in the tooltip.

I don't know what's the problem here.

vlTooltip should supplement format for a field from the Vega-Lite spec only if (1) the field is in the list or (2) showUnspecifiedFields is true.

If you mean user might want to manually configure format even though the field is not a part of the list, that case also doesn't happen because you would only want to configure the displayed field.

@light-and-salt
Copy link
Contributor Author

This looks very complicated and it's not really an array?

I think for bin it can be an array because we know the underscore prefix is always going to be 'bin_'.

For timeUnit we also want to know what timeUnit it is because item.datum could be month_field, yearmonth_field, year_field etc.

Similarly, for aggregate we want to know what aggregation it is, as item.datum could give me mean_field, sum_field, min_field etc.

@domoritz
Copy link
Member

I agree with @kanitw that we don't need to separate field and field configs if we have the showUnspecifiedFields. I also don't see why we need timeUnit because the vl method should just pass the right field to the vg.tooltip rather than relying on the receiving method to do the translation.

@light-and-salt
Copy link
Contributor Author

OK, I'm happy to go with the combined way with showUnspecifiedFields.

@light-and-salt
Copy link
Contributor Author

@domoritz I think the user won't have to specify timeUnit. The tooltip plugin will read the vlSpec and supplement timeUnit. We only need the timeUnit to recognize that a user-provided field name is a valid field name. We don't want the user to provide underscore prefixed field names but we have underscore prefixed field names at run time.

@light-and-salt
Copy link
Contributor Author

Instead of fields[] and showUnspecifiedFields : boolean, I'm also thinking about customFields[] and showAllFields : boolean. How do you guys feel about that?

@light-and-salt
Copy link
Contributor Author

New options template (Apr 12 version):

var options = 
{
    showAllFields: true | false | undefined, // true by default, undefined equals true
    customFields: [
      {
         field: 'field1', // user provides (vlSpec supplements when showAllFields === true || undefined)
         title: ...,                // user provides, vlSpec supplements
         formatType: ...,           // user provides, vlSpec supplements
         format: string-specifier,  // user provides, vlSpec supplements

         aggregate: ['max', 'sum'], // vlSpec supplements
         timeUnit: 'yearmonth'      // vlSpec supplements
         bin: true | false          // vlSpec supplements
      },
      ...
    }],

    colorTheme: 'light' | 'dark'
};

// When options is undefined, tooltip creates an options object, sets showAllFields to true, 
// and supplement customFields using information from vlSpec.

// When showAllFields is true, tooltip supplements customFields using vlSpec, adding new fields
// to customFields when necessary

// When showAllFields is false, tooltip only supplements existing fields in customFields

// When showAllFields is undefined, tooltip sets it to true, and supplement customFields using vlSpec,
// adding new fields to customFields when necessary

@kanitw
Copy link
Member

kanitw commented Apr 13, 2016

customFields[]

I would say just name it fields -- the fact that users are providing it explicitly is already custom. No need to make it verbose.

For aggregate, just make it aggregate value. If you have two fieldDefs with the same field but different aggregation, it should be repeated twice here. This way, each item in fields is simply a FieldDef and is more consistent to read.

(FieldDef already have title and can have format and formatType for consistency)

@light-and-salt
Copy link
Contributor Author

For aggregate, just make it aggregate value. If you have two fieldDefs with the same field but different aggregation, it should be repeated twice here. This way, each item in fields is simply a FieldDef and is more consistent to read.

You mean it's possible to have repeated fields like below?

var options = 
{
    fields: [
      {
         field: 'field1',
         aggregate: 'sum' // vlSpec supplements
      },
      {
        field: 'field1',
        aggregate: 'max' // vlSpec supplements
      }
    }]
};

And this is what fieldDef does when a field has multiple aggregations? Could you tell me why fieldDef is designed that way?

@kanitw
Copy link
Member

kanitw commented Apr 13, 2016

Yes.

Could you tell me why fieldDef is designed that way?

Because different aggregation function produces multiple fields in the output aggregated table.
You can't map both "min" and "max" to single x encoding of a mark without composition (e.g., layering).

@light-and-salt
Copy link
Contributor Author

FieldDef already have title

I know from Vega-Lite code that there is FieldDef.title, but with the tooltip examples I have, I don't see any FieldDef.title. I only see FieldDef.legend.title and FieldDef.axis.title. @domoritz and I talked about this at the end of last quarter. FieldDef.title should be a shorthand for the titles specified at lower levels, right? Is that something that needs to be done?

@domoritz
Copy link
Member

We fixed that just two days ago vega/vega-lite#1295. You might have to pull to see it.

@light-and-salt
Copy link
Contributor Author

@kanitw I think I still prefer having aggregate as an array. Unlike Vega-Lite, which needs to actually perform the aggregation operation functions on fields, in tooltip I only need the names of the aggregations so that I can do name-matching. So it's easier to have all the aggregation functions in one place. It would be easier to tell whether a field name is valid in that way.

Also, when showAllFields is false, I will go through the list of fields to get the list of custom fields. If there are repeated fields in the list because of multiple aggregation functions I will have to combine them anyway. In general we only want to show one row per field in the tooltip, even if the field has multiple aggregations.

What do you think?

@kanitw
Copy link
Member

kanitw commented Apr 13, 2016

I disagree.

In general we only want to show one row per field in the tooltip, even if the field has multiple aggregations.

Your output SHOULD display multiple aggregate fields (e.g., min and max) as separate rows/fields in the output tooltip. Showing only one would only confuse users.

Thus, making the API clear that you're displaying multiple aggregate fields is better (esp. if showAllFields is false).

@light-and-salt
Copy link
Contributor Author

@domoritz That's good to know.

I built the latest vega-lite code and used it locally in tooltip. I'm using vl.spec.fieldDefs(vlSpec) where vlSpec is stacked_bar_weather.json. In my output I have fieldDef.title as undefined but fieldDef.legend.title as Weather type

screen shot 2016-04-13 at 11 35 15 am

@kanitw kanitw changed the title Underscore prefix API Option Apr 13, 2016
@light-and-salt
Copy link
Contributor Author

Your output SHOULD display multiple aggregate fields (e.g., min and max) as separate rows/fields in the output tooltip.

Ah, I see... Now I agree with you that we should have repeated fields in options for fields with multiple aggregations.

@light-and-salt
Copy link
Contributor Author

Would a user ever need to specify aggregate in options? For example, if a field has two aggregations min and max, should the user provide:

var options = 
{
    showAllFields: false,
    fields: [
    {
        field: 'field1',
        aggregate: 'min'
    },
    {
        field: 'field1',
        aggregate: 'max'
    }
    ]
}

to explicitly specify she wants to show both min_field1 and max_field1.

Or, should a user just provide:

var options = 
{
    showAllFields: false,
    fields: [
    {
        field: 'field1'
    }
    ]
}

to more vaguely say that she wants to show field1. And the tooltip supplements the options with min and max aggregations.

I guess the first (more explicit) way is better? Because the user can, for example, show only min_field1 and not max_field1? Is this a valid scenario? In the second way, if the user specified field1 then we always show both min_field1 and max_field1. There is no further customization in which aggregated field to show in the tooltip.

@light-and-salt
Copy link
Contributor Author

This is part of PR #16. Here's how options look like now:

var options =
{
    colorTheme: 'light' | 'dark',
    showAllFields: true | false | undefined,
    fields: [
      {
         field: 'field1', // user provides, vlSpec supplements underscore prefixes
         title: 'Field One',
         formatType: 'time' | 'number' | 'string',          
         format: string-specifier,
         aggregate: operation,
      },
      ...
    }]
};

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

No branches or pull requests

3 participants