add lowerPercentile & upperPercentile back to hexagonLayer #470
Conversation
There was a problem hiding this comment.
Need a CHANGELOG entry - if so feel free to remove the deprecation warnings (see comments).
Q: Does quantizeScale depend on an additional d3-module being imported by deck.gl? This could be the reason this was pushed out previously? I don't see a package.json change but wonder if one is required.
| @@ -70,13 +70,13 @@ export default class HexagonCellLayer extends Layer { | |||
| constructor(props) { | |||
| let missingProps = false; | |||
| if (!props.hexagonVertices && (!props.radius || !Number.isFinite(props.angle))) { | |||
| log.once(0, 'HexagonLayer: Either hexagonVertices or radius and angel are ' + | |||
| log.once(0, 'HexagonCellLayer: Either hexagonVertices or radius and angel are ' + | |||
| 'needed to calculate primitive hexagon.'); | ||
| missingProps = true; | ||
|
|
||
| } else if (props.hexagonVertices && (!Array.isArray(props.hexagonVertices) || | ||
| props.hexagonVertices.length < 6)) { | ||
| log.once(0, 'HexagonLayer: HexagonVertices needs to be an array of 6 points'); | ||
| log.once(0, 'HexagonCellLayer: HexagonVertices needs to be an array of 6 points'); |
There was a problem hiding this comment.
camelCase? hexagonVertices
| Math.max.apply(null, hexagons.map(bin => bin.points.length)) | ||
| ]; | ||
| function _percentileChanged(oldProps, props) { | ||
| return oldProps.lowerPercentile !== props.lowerPercentile || |
There was a problem hiding this comment.
This is a total nit, feel free to ignore: Is it just me that thinks the order of comparison reads better as props vs oldProps rather than the reverse? i.e. if (props.XYZ !== oldProps.XYZ)
| } | ||
|
|
||
| export default class HexagonLayer extends Layer { | ||
| constructor(props) { | ||
| if (!props.radius) { | ||
| log.once(0, 'PointDensityHexagonLayer: radius in meter is needed to aggregate points into ' + | ||
| log.once(0, 'HexagonLayer: radius in meter is needed to aggregate points into ' + |
There was a problem hiding this comment.
I think a deprecation warning is only needed if this layer was officially supported in another release, and this is a new layer.
People using RC will have to read the release notes - which seem to be missing here.
There was a problem hiding this comment.
This is not a deprecation warning. I simply changed the name of the layer after we changed PointDensityHexagonLayer -> HexagonLayer
| 'hexagonal bins, Now using 1000 meter as default'); | ||
|
|
||
| props.radius = defaultProps.radius; | ||
| } | ||
|
|
||
| if (Number.isFinite(props.upperPercentile) && |
There was a problem hiding this comment.
Just a thought: These checks adds a lot lines of code to the layer. Wonder if we should add and use some basic layer prop handlers like
https://github.com/uber/deck.gl/blob/master/src/lib/layer.js#L540
Also we sometimes throw errors and sometimes warn and set defaults - not sure if this is worthy of a paragraph in our API design recommendations doc?
There was a problem hiding this comment.
I agree we should have a more formal way of checking props. At the moment, this can't be called by checkRequiredProps. because they are not required props. I only check if those props are valid when user provide them.
Maybe we should create a ticket for better prop checking?
|
|
||
| const colorDomain = this.props.colorDomain || valueDomain; | ||
| const count = cell.points.length; | ||
| const color = quantizeScale(colorDomain, colorRange, count); |
There was a problem hiding this comment.
Does quantizeScale depend on an additional d3-module being imported by deck.gl? This could be the reason this was pushed out previously?
There was a problem hiding this comment.
quantizeScale is implemented from scratch in utils/scale-util. We are not depend on d3 for this
| @@ -77,9 +103,16 @@ export default class HexagonLayer extends Layer { | |||
| const {viewport} = this.context; | |||
|
|
|||
| const hexagons = hexagonAggregator(this.props, viewport); | |||
| const countRange = _getCountRange(hexagons); | |||
| const sortedCounts = new SortedBins(hexagons); | |||
There was a problem hiding this comment.
The name of this class doesn't feel quite right, but not sure I can't come up with anything better. BinSorter?
src/utils/sorted-bins.js
Outdated
| @@ -18,6 +18,7 @@ export default class SortedBins { | |||
| /** | |||
| * Get an array of object with sorted count and index of bins | |||
| * @param {Number} lower | |||
There was a problem hiding this comment.
Nit: This doesn't correctly document the params (looks like f(lower, upper)) . But probably no need to change this now. We'll probably move to something like flow type annotation comments in the future. A more correct version of JSDoc would probably look like:
@param {Number[]} range
@param {Number} range[0] - lower bound
...
heshan0131
force-pushed
the
hexagon-grid-percentile
branch
from
b84c7fe to
1baedb4
Compare
* add lowerPercentile & upperPercentile back to hexagonLayer * update based on comments * add to CHANGELOG
No description provided.