@jfirebaugh Discrete scales should be explicitly denoted by their rounding parameter

Ok, now I pinky swear to not make any more commits to this PR ☺️

Nice work. There are a few stylistic nits that I'll give line-by-line comments for, and a few architectural issues.

I still think we will want scales to be explicitly typed. I'll write up why in detail later.

The API here has some features that I'm not sure are justified in terms of their cost-benefit:

• Continued support of legacy format. Let's pull the support for this into a separate function that can migrate from old to new. That will be useful in the style migration script, and allow the core code to process one consistent format only.
• Multiple ways to pass parameters, i.e. scale(0) versus scale({$zoom: 0}) and scale({mapbox: 1}, {mapbox: 3}), scale(3, {mapbox: 1}), and scale({mapbox: 1}, 3). I think we should break backward compatibility in order to standardize on a single convention. See below for thoughts on an optimal convention.
• Rounding modes. As far as I know, the single rounding behavior that piecewise-constant functions provides has been entirely sufficient. Consider what value none, floor, and ceiling provide, and weigh that against the costs of added code (in both js and native), tests, documentation, and editor UI.

Parameter passing and partial evaluation

See the discussion thread on mapbox/mapbox-gl-js#1267. I think we want this module to support partial evaluation of functions over what you elsewhere called "Global Attributes", which is a term I like.

Example of proposed API use:

var scale = MapboxGLScale({
  domain: [1, 3],
  range: [2, 6],
  property: '$zoom'
});

var partial1 = scale.globalEval({zoom: 1});
partial1({mapbox: 2}); // 2
partial1({mapbox: 3}); // 2

var partial2 = scale.globalEval({zoom: 2});
partial2({mapbox: 2}); // 4
partial2({mapbox: 3}); // 4

// Potential shorthand notation - we may not need this
scale({zoom: 1}, {mapbox: 2}); // 2
scale({zoom: 2}, {mapbox: 2}); // 4

This is nice because we can optimize the result of scale.globalEval to be a function that ignores its argument and returns a constant value when the scale does not depend on feature properties.

I still think we will want scales to be explicitly typed. I'll write up why in detail later.

I have definitely come to agree with you here. I think specifying a rounding parameter is equivalent to and more desirable than a type parameter, in the world of discrete and continuous scales. My intentions here are to

• create conceptually continuous parameterized design space for users of the editor GUI

• avoid separate code paths for different types until we absolutely need to

• avoid supporting different features for different types until we absolutely need to

• allow continuous inputs to be mapped to discrete outputs without the complexity of d3 scales

  Continued support of legacy format. Let's pull the support for this into a separate function that can migrate from old to new. That will be useful in the style migration script, and allow the core code to process one consistent format only.

Ok! I'll do that.

Multiple ways to pass parameters, i.e. scale(0) versus scale({$zoom: 0}) and scale({mapbox: 1}, {mapbox: 3}), scale(3, {mapbox: 1}), and scale({mapbox: 1}, 3). I think we should break backward compatibility in order to standardize on a single convention. See below for thoughts on an optimal convention.

I'm on board with dropping support for numeric arguments, automatically converted to {$zoom: ...} objects.

The main goal with the scale(object, object, ...) syntax was avoiding unnecessary object construction and cloning. If we move to partial evaluation, that problem goes away in the short term (no need to add $zoom to feature attributes).

Rounding modes. As far as I know, the single rounding behavior that piecewise-constant functions provides has been entirely sufficient. Consider what value none, floor, and ceiling provide, and weigh that against the costs of added code (in both js and native), tests, documentation, and editor UI.

Dropping the ceiling mode is fine by me.

The floor vs normal modes are intended to be the way we differentiate between continuous and discrete function types, so I'd like to keep those or replace them with a type parameter.

See the discussion thread on mapbox/mapbox-gl-js#1267. I think we want this module to support partial evaluation of functions over what you elsewhere called "Global Attributes", which is a term I like.

Yup, this seems pretty obvious in retrospect.

Next Steps

• Move legacy support into a separate function
• Drop support for numerical $zoom arguments
• Refactor to use partial evaluation for global attributes / feature attributes
• Remove the ceiling rounding mode.
• Add support + tests for string and boolean valued domains
• Move validation into a separate function

API proposal:

module.exports = create;
module.exports.migrate = function() { ... }; // migration function, to be used by gl-style-migrate
module.exports.validate = function() { ... }; // validation function, to be used by gl-style-validate and internal validation

Next Steps

👍 -- perfect

@jfirebaugh 🎁

@jfirebaugh Is this ready to go?

Ok! I made the changes you requested as well as

• removing functionality that's now duplicated in gl-style-spec
• adding an isConstant property on all scales which will help @ansis in a way to tell if a scale depends on feature properties #3

@jfirebaugh this is ready for 👀

!

@jfirebaugh Are we going to break anything by merging this into master right now? Maybe we should keep this in a separate branch for now or pin upstream dependencies to a specific tag of this repository

Merging is safe. Dependent modules are using a versioned release from npm, so they will be fine. If integration in gl-js goes well, we'll publish the changes as v2.0.0.

Ok, just bumped the version. Will merge as soon as CI passes.