Skip to content
This repository has been archived by the owner on Apr 10, 2018. It is now read-only.

Specify a default for property functions when property value is undefined #480

Closed
lbud opened this issue Aug 16, 2016 · 12 comments
Closed
Assignees

Comments

@lbud
Copy link
Contributor

lbud commented Aug 16, 2016

In the mapbox-gl-function feature-defaults branch I'm adding reference as an argument so that when an input to a property function is undefined, it defaults to the spec default. However, it seems to me there should also be a way to specify a user-defined default for this behavior. Does it seem reasonable to add an optional key in a property function for default, like

extrusion-height: {
    stops: [[0, 0], [1000, 3000]],
    type: "exponential",
    property: "levels",
    default: 20
}

(so any extrusion without a levels property will have a height of 20, rather than the spec-defined default of 10)?

cc @lucaswoj @mollymerp @mourner

@mourner
Copy link
Member

mourner commented Aug 16, 2016

I like the idea. default property name can be confusing though, since it can be confused with extrusion-height default, not the default for the input property of a function. Alternatives? propertyDefault?

@lbud
Copy link
Contributor Author

lbud commented Aug 16, 2016

propertyDefault (or property-default — hyphens would be more consistent in this spec, I think) sounds good to me.

@jfirebaugh
Copy link
Contributor

Could we unify the syntax with the proposals for token defaults in #104?

@lbud
Copy link
Contributor Author

lbud commented Aug 26, 2016

@jfirebaugh do you mean something like

extrusion-height: {
    stops: [[0, 0], [1000, 3000]],
    type: "exponential",
    property: "levels:20"
}

? (It feels weird to use a number in a string there…)

@jfirebaugh
Copy link
Contributor

No, I mean something akin to the syntaxes proposed in #362 (comment) or #104 (comment), like "property": {"ref": "levels", "default": 20} or "property": [{"ref": "levels"}, 20].

I think at this point we need to have a vision that unifies #480, #104, and #47 or the function syntax will wind up becoming a mess of ad hoc additions.

@lbud
Copy link
Contributor Author

lbud commented Sep 16, 2016

Interestingly, I realized that because of the way evaluateExponentialFunction picks a function stop — iterating over them, waiting for either a break statement or the end — we can exploit it to set, essentially, a default:

{
    "stops": [[0, 0], [1000, 1000], ["other", 20]]
    "property": "height"
}

I don't feel like this is really the intended behavior or a clean solution, buuuut… 💭

@jfirebaugh @lucaswoj

@lbud
Copy link
Contributor Author

lbud commented Sep 19, 2016

Actually, my above comment is not true — it was working in my debugging but now I'm creating demos and realizing validation throws out non-number stops in number functions, and non-ascending-order stops (so -1 doesn't work). I can still achieve this effect by ensuring that the second to last stop is a number beyond what would ever exist in the data, and then making the next number even larger: stops: [[0, 0], [1000, 1000], [2000, 20]] but this feels even more unclean.

@lucaswoj
Copy link

lucaswoj commented Sep 20, 2016

I think there are two distinct cases being discussed in this thread:

1. Specify a function input value when the property is undefined

This is being discussed in #104. I will weigh in on that ticket.

2. Specify a function output value when the property is undefined

"extrusion-height": {
    stops: [[0, 0], [1000, 3000]],
    type: "exponential",
    property: "levels",
    default: 20
}

any extrusion without a levels property will have a height of 20, rather than the spec-defined default of 10

In some cases, we can take care of case 2 with a carefully constructed value and the case 1 feature. In other cases, doing so is impossible or mathematically tricky. For example:

"fill-color": {
    stops: [["scrub", "yellow"], ["industrial", "purple"], ...],
    default: "white",
    type: "categorical",
    property: "class"
}

We should have a primitive for this case. @lbud's syntax proposal is 👍

@jfirebaugh
Copy link
Contributor

@lucaswoj Good clarification -- let's focus this issue on adding the ability to provide an output value for inputs which are not otherwise in the domain of the function.

An input value of "undefined", when the property is not present on a given feature, is a common special case. But a categorical function which doesn't wish to specify a stop for every possible value is another use case.

Then to go back to @mourner's first comment:

default property name can be confusing though, since it can be confused with extrusion-height default, not the default for the input property of a function

If we are talking about extrusion-height output default, not a default input, I think default is fine -- it's the same concept as a default label in a case statement, for example.

@davidtheclark
Copy link
Contributor

davidtheclark commented Dec 3, 2016

A clarification question: By "undefined" are we referring to both undefined and null? Or is null going to have to be a separate conversation?

@jfirebaugh
Copy link
Contributor

A clarification question: By "undefined" are we referring to both undefined and null? Or is null going to have to be a separate conversation?

We're not referring to either a literal undefined or null input value, but rather to the case where the feature properties have no key-value pair for the desired property at all.

null is a separate conversation. Functions and filters don't currently have a notion of null values, largely because neither does the vector tile specification.

@lucaswoj
Copy link

lucaswoj commented Feb 1, 2017

This issue was moved to mapbox/mapbox-gl-js#4124

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants