-
Notifications
You must be signed in to change notification settings - Fork 942
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
New module @turf/interpolate
(proposal)
#832
Conversation
added setColor function in test.js; removed unused dependences; modified test input;
@turf/interpolate
@turf/interpolate
(proposal)
@DenisCarriere I don't know why the test pass in my computer, but not in Travis... 😞 |
I'll comment more later on this weekend, however for the Travis build fail, I'm guessing it's a decimal precision error, even using Node 6 vs. 7 you could have slight differences. Solution would be to use the Quick glance, this looks like an awesome module! |
packages/turf-interpolate/test.js
Outdated
const {property, cellSize, units, weight} = geojson.properties; | ||
|
||
const grid = interpolate(geojson, cellSize, property, units, weight); | ||
const result = colorize(grid, property); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added @turf/truncate
to results (reduce coordinates to 6 decimals).
Node 7 & 8 were passing, but Node 6 was failing (0.0000001 of a difference).
packages/turf-interpolate/index.d.ts
Outdated
@@ -0,0 +1,10 @@ | |||
/// <reference types="geojson" /> | |||
|
|||
import {Points} from '@turf/helpers'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since @turf/helpers
is not required in the main package.json
, it might be better to include the Full GeoJSON typescript definition instead of importing it (in case the dependencies wouldn't include @turf/helpers
).
type Point = GeoJSON.FeatureCollection<GeoJSON.Point>;
packages/turf-interpolate/index.d.ts
Outdated
/** | ||
* http://turfjs.org/docs/#interpolate | ||
*/ | ||
declare function interpolate(points: Points, cellSize: number, property?: number, units?: string, weight?:number): Points; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
property?: number
should be a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contradicts my previous comment, but units?: string
could use Units
instead of string
.
import {Units} from '@turf/helpers'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah! nice to know, I didn't noticed the types in @turf/helpers
👍
packages/turf-interpolate/index.js
Outdated
if (!points) throw new Error('points is required'); | ||
collectionOf(points, 'Point', 'input must contain Points'); | ||
if (!cellSize) throw new Error('cellSize is required'); | ||
if (cellSize === undefined || cellSize === null) throw new Error('cellWidth is required'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to assume this is a duplicate of cellSize
.
CellSize
cannot be 0, correct?
if (!cellSize) throw new Error('cellSize is required');
if (cellSize === undefined || cellSize === null) throw new Error('cellWidth is required');
Question: Should we add a CC: @stebogit |
- Add tests for throwing errors - Update Readme
@@ -0,0 +1,16 @@ | |||
import * as linearc from '../index' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️ Looks like this file came from the previous repo 🤣
I've been putting types.ts
at the root directory that way we don't forget it.
One day... all modules will have types.ts
to test for the Typescript definitions.
So far this one looks like this:
import * as interpolate from './';
import {point, featureCollection} from '@turf/helpers';
const cellSize = 1;
const property = 'pressure';
const weight = 0.5;
const units = 'miles';
const points = featureCollection([
point([1, 2]),
point([12, 13]),
point([23, 22]),
]);
const grid = interpolate(points, cellSize, property, units, weight);
grid.features[0].properties.pressure;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me!
|
Agreed, however the only difference with CC: @stebogit |
@DenisCarriere I don't see the number of decimals as an issue, the user can simply round it, if necessary, to the desired precision when using the interpolated value(s) |
👍 That works, yea it's not really an issue. |
@stebogit Have a quick glance at it before we merge, I'm done with any edits. |
👍 Agreed, maybe once we bump up to a major release v5.0 we could do that. I also prefer the term CC: @morganherlocker Thoughts? |
@DenisCarriere yes, having this module doing both is really easy (I'll implement that).
👍 we could add an When I implemented this I was wondering if @morganherlocker had actually a different idea/algorithm of interpolation in mind when suggesting this module |
BTW anybody knows if there is a more appropriate default value for |
@DenisCarriere I added the |
👍 I like the option of defining the different Defining a default param should be fine, we can use /**
* @param {string} [gridType='square'] defines the output format based on a Grid Type (options: 'square' | 'point') |
Last edits before we merge:
|
updated docs, yarn.lock, bench, index.d.ts; added tests;
@DenisCarriere good to merge I believe. |
Implements #24
JSDocs
Example
zValue
as 3rd coordinate ifproperty
is not defined.@turf/idw
?@DenisCarriere following #829 I thought maybe it would make sense to implement this module with the IDW method. In fact I don't really know any other interpolation methods, but I'm open to suggestions.
However, this differs from
@turf/idw
only in outputting apointGrid
instead of asquareGrid
. Does it make sense to have two modules here?If not I would rather merge the two in a single
turf-interpolate
(I think is more clear to an average user like me, who ignored the meaning ofIDW
since this morning...) and add to the module anoutputType
parameter (default to one of the two).@bakerac4 maybe you want to try this module out and see if it fits your needs.