- Add coverage percentiles getColorValue prop to grid layer #614

heshan0131 · 2017-04-29T00:51:04Z

To make grid layer consistent with hexagon layer, add coverage, lowerPercentile, upperPercentile and getColorValue to layer prop
Fix bug in hex _onGetSublayerColor, use bin.value instead of bin.points.length to calculate color

ibgreen · 2017-05-01T18:42:50Z

docs/layers/grid-layer.md

@@ -62,6 +62,23 @@ Color scale domain, default is set to the range of point counts in each cell.
 Color ranges as an array of colors formatted as `[255, 255, 255]`. Default is
 [colorbrewer](http://colorbrewer2.org/#type=sequential&scheme=YlOrRd&n=6) `6-class YlOrRd`.

+##### `getColorValue` (Function, optional)


Is there a reason not to just call this getValue?

The description can explain that this is used to calculate value.

##### `getColorValue` (Function, optional)

Note to self: It would be nice to be able to show more clearly the expected return type of the accessors (in this case number)

Because the value is only used to calculate color, not elevation, so I thought it would be nice to make it clear in the name

And in the future when I add another feature to calculate elevation based on points value I can use getElevationValue

Sounds like we are establishing a new proposed pattern here. Maybe add a section with proposed rules to API GUIDELINES

ibgreen · 2017-05-01T18:43:44Z

docs/layers/grid-layer.md

+- Default: `points => points.length`
+
+`getColorValue` is the accessor function to get the value that cell color is based on. 
+It takes an array of points inside each cell as arguments, returns a value. For example, 


"returns a value" => "returns a number"?

ibgreen · 2017-05-01T18:45:40Z

docs/layers/grid-layer.md

+
+- Default: `1`
+
+Cell size multiplier, clamped between 0 - 1. The final size of cell


Specify whether coverage is linear or area based? I assume this is just a clamped radiusScale factor, i.e. linear.

ibgreen · 2017-05-01T18:47:05Z

docs/layers/hexagon-layer.md

-Filter bins and re-calculate color by `upperPercentile`. Hexagons with counts
-bigger than the upperPercentile counts will be hidden.
+Filter bins and re-calculate color by `upperPercentile`. Hexagons with value
+larger than the upperPercentile will be hidden.


Wouldn't it be cool if they were just made really transparent / ghosted?

This reason of doing this is to filter out really big numbers and re calculate color. When colors for are recalculated, the larger bins that fall outside the percentile range will not have accurate color any more, so we have to hide it. Also if we want to display transparency, we have to always enable depthTest

the larger bins that fall outside the percentile range will not have accurate color any more, so we have to hide i

I thought we could make the filtered out hexes a white transparent color - i.e. they would desaturate and just become ghosts, but could still retain their elevation and give the user a feeling for what he has filtered out.

Also if we want to display transparency, we have to always enable depthTest.

I guess this could be an optional feature. A prop...

ibgreen · 2017-05-01T18:49:11Z

examples/layer-browser/src/examples/core-layers.js

+function getMean(pts, key) {
+  const filtered = pts.filter(pt => Number.isFinite(pt[key]));
+
+  return filtered.length ?


Why the check?

[].reduce((acc, elt) => acc + elt, 0) => 0

if value is null or undefined, we do not include it when calculate Mean, so we need to get the total number of elements that are not null to divide the sum by...

if value is null or undefined, we do not include it when calculate Mean, so we need to get the total number of elements that are not null to divide the sum by

Then the comparison should not use filtered.length, but just filtered right? Perhaps a good opportunity to add a test case that triggers this bug, since you are working on coverage anyway?

ibgreen · 2017-05-01T18:50:58Z

src/layers/core/grid-layer/grid-layer.js

@@ -25,18 +25,20 @@ import {pointToDensityGridData} from './grid-aggregator';
 import {linearScale, quantizeScale} from '../../../utils/scale-utils';
 import {defaultColorRange} from '../../../utils/color-utils';

-const defaultCellSize = 1000;


+1, looks cleaner after the change

ibgreen · 2017-05-01T18:53:40Z

src/layers/core/grid-layer/grid-layer.js

+}
+
+function _needsReSortBins(oldProps, props) {
+  return oldProps.getColorValue !== props.getColorValue;


I would recommend giving this update criterion another round of thought, as it is inconsistent with updateTriggers which we use to make sure we don't update when functions change.

This is to handle the common cases of ad-hoc functions and bound functions:

new Layer({ getColorValue: () => {}, // new function every render, getColorValue: this._getColorValue.bind(this), // new function every render, });

I added comment to layer doc.
If people pass in a new function every time, the layer is just going to sort the bins on every render, which won't result in error showing colors. But if we don't check the function change, and not re sort the bins when it actually changed. we have no way to detect whether some one pass in a new function to calculate color value.

we have no way to detect whether some one pass in a new function to calculate color value.

Unless we expand updateTriggers to cover this case (but that would probably require a discussion and a separate PR).

OK for now, but I think it is a potential API design issue that we have different behavior for accessors and other functions. We might want to call it out in one of our documents (possibly API guidelines if you decided to update there).

ibgreen · 2017-05-01T18:54:43Z

src/layers/core/grid-layer/grid-layer.js


-      Object.assign(this.state, {layerData, countRange});
+      this.onPercentileChange();


onPercentileChanged is quite suggestive of a callback (i.e. calling a function from another component). I'd change this to percenticlesChanged(); or similar

ibgreen · 2017-05-01T18:58:11Z

src/layers/core/grid-layer/grid-layer.js


-    return quantizeScale(colorDomain, colorRange, cell.count);
+    return color.concat([alpha]);


Not sure how frequently this is called, but cloning arrays (Array.concat) seems like a recipe for bad performance. Can quantizeScale just take an alpha argument?

ibgreen · 2017-05-01T18:59:42Z

src/layers/core/hexagon-layer/hexagon-layer.js

@@ -171,34 +170,34 @@ export default class HexagonLayer extends CompositeLayer {
    };
  }

-  _onPercentileChange() {
+  onPercentileChange() {


Are these callbacks? If not, I'd avoid the on... naming convention.

coveralls · 2017-05-01T21:25:00Z

Changes Unknown when pulling 30b5858 on grid-colorValue-percentile into ** on master**.

coveralls · 2017-05-01T21:52:29Z

Changes Unknown when pulling 171edd5 on grid-colorValue-percentile into ** on master**.

* add getColorValue percentile coverage to grid layer * add layer tests * added grid/hex layer test * fix typo * reset yarn.lock * address comments * replace concat

heshan0131 added 3 commits April 29, 2017 12:53

add getColorValue percentile coverage to grid layer

ffc7536

add layer tests

53a7793

added grid/hex layer test

1dbcc84

heshan0131 force-pushed the grid-colorValue-percentile branch from bb58330 to 1dbcc84 Compare April 30, 2017 05:29

heshan0131 added 2 commits April 30, 2017 14:23

fix typo

f6fd1cc

reset yarn.lock

fb308ad

heshan0131 requested a review from ibgreen May 1, 2017 16:45

ibgreen approved these changes May 1, 2017

View reviewed changes

address comments

30b5858

replace concat

171edd5

heshan0131 merged commit d76e820 into master May 1, 2017

balthazar deleted the grid-colorValue-percentile branch June 15, 2017 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

- Add coverage percentiles getColorValue prop to grid layer #614

- Add coverage percentiles getColorValue prop to grid layer #614

heshan0131 commented Apr 29, 2017

ibgreen May 1, 2017

heshan0131 May 1, 2017

heshan0131 May 1, 2017

ibgreen May 1, 2017

ibgreen May 1, 2017

ibgreen May 1, 2017

ibgreen May 1, 2017

heshan0131 May 1, 2017 •

edited

Loading

ibgreen May 1, 2017

ibgreen May 1, 2017

heshan0131 May 1, 2017

ibgreen May 1, 2017

ibgreen May 1, 2017

ibgreen May 1, 2017

heshan0131 May 1, 2017

ibgreen May 1, 2017

ibgreen May 1, 2017

ibgreen May 1, 2017

ibgreen May 1, 2017

coveralls commented May 1, 2017 •

edited

Loading

coveralls commented May 1, 2017 •

edited

Loading


		- Default: `1`

		Cell size multiplier, clamped between 0 - 1. The final size of cell


		Object.assign(this.state, {layerData, countRange});
		this.onPercentileChange();


		return quantizeScale(colorDomain, colorRange, cell.count);
		return color.concat([alpha]);

- Add coverage percentiles getColorValue prop to grid layer #614

- Add coverage percentiles getColorValue prop to grid layer #614

Conversation

heshan0131 commented Apr 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heshan0131 May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 1, 2017 • edited Loading

coveralls commented May 1, 2017 • edited Loading

heshan0131 May 1, 2017 •

edited

Loading

coveralls commented May 1, 2017 •

edited

Loading

coveralls commented May 1, 2017 •

edited

Loading