Skip to content
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

Revamp grid modules #1029

Merged
merged 30 commits into from
Oct 25, 2017
Merged

Revamp grid modules #1029

merged 30 commits into from
Oct 25, 2017

Conversation

stebogit
Copy link
Collaborator

@stebogit stebogit commented Oct 18, 2017

Ref #1019

New ⚠️

  • Modules which input Point data can simply use an Array<number>.
  • Optional parameters should be declared initially options.mask => mask
  • options.mask should have (Multi)Polygon geometry type validation.
  • Add @turf/truncate to output test results to prevent any precision decimal errors.

New - Improvements ⚠️

  • Add translate properties to grid features (options.properties)
  • Add mask to each grid module (options.mask)
  • Use @turf/boolean-within or @turf/intersect for options.mask

Update fixtures for

  • @turf/interpolate
  • @turf/transform-scale

@stebogit
Copy link
Collaborator Author

@DenisCarriere I noticed all the grid have projection distortion because of the way the grid elements are created:
screen shot 2017-10-17 at 10 45 24 pm

screen shot 2017-10-17 at 10 47 39 pm

screen shot 2017-10-18 at 12 17 33 am

Should we modify that and have Mercator-projected grids or we consider these correct outputs?

@DenisCarriere
Copy link
Member

DenisCarriere commented Oct 18, 2017

Should we modify that and have Mercator-projected grids or we consider these correct outputs

👍 Yes I think we should make it Mercator friendly since 98% of users will be using a Leaflet type map to display their geojson.

@DenisCarriere
Copy link
Member

🤔 ... @stebogit might be best to use a new branch.. lots of conflicts and might be just easier to start a new PR based on the master branch.

I think these conflicts are caused from the @turf/distance earth radius update.

@stebogit
Copy link
Collaborator Author

@DenisCarriere I'm not sure what's the correct type definition for mask here, kind of confused between Polygon and Feature<Polygon> 🤔 :

screen shot 2017-10-19 at 12 17 33 am

@DenisCarriere
Copy link
Member

@stebogit Sorry been busy the past days, I'm in Boulder at SOTM now.

The difference between Polygon & Feature<Polygon>:

  • Polygon is the same as Geometry<Polygon> which would look like this:
{
  type: "Polygon",
  coordinates: Array<number>
}
  • Feature<Polygon> is the Feature wrapped with the Polygon Geometry.

You can always look at the @types/geojson file to decipher how the GeoJSON.Polygon & GeoJSON.Feature<GeoJSON.Polygon> have been created:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/geojson/index.d.ts#L68-L71

@turf/helpers tries to replicate an exact match of the GeoJSON type definition, except you don't have to add GeoJSON.<type> you can simply just import it from @turf/helpers.

import {Polygon, Feature} from '@turf/helpers'

import pointGrid from './';

const cellSide = 50;
const bbox: BBox = [-95, 30, -85, 40];
const poly: Polygon = polygon([[[20, 30], [10, 10], [20, 20], [20, 30]]]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stebogit This is my bad... Polygon was short for Feature<Polygon>, however now Polygon from @turf/helpers is now equivalent to Geometry<Polygon>.

The reason for the error is because mask doesn't handle adding Geometry<Polygon> (which should be a valid input, both Geometry & Feature).

cellSide: number,
options?: {
units?: Units,
properties?: Properties,
bboxIsMask?: boolean;
mask?: Polygon|MultiPolygon;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct definition would be:

To support both Geometry<Polygon|MultiPolygon> & Feature<Polygon|MultiPolygon>

export default function pointGrid(
    bbox: BBox,
    cellSide: number,
    options?: {
        units?: Units,
        properties?: Properties,
        mask?: Feature<Polygon | MultiPolygon> | Polygon | MultiPolygon;
    }
): FeatureCollection<Point>;

* @param {Object} [options={}] Optional parameters
* @param {string} [options.units="kilometers"] used in calculating cellSide, can be degrees, radians, miles, or kilometers
* @param {number} [options.bboxIsMask=false] if true, and bbox is a Polygon or MultiPolygon, the grid Point will be created
* @param {Feature<Polygon|MultiPolygon>} [options.mask=undefined] if passed a Polygon or MultiPolygon, the grid Points will be created only inside it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to define the option as undefined, [options.mask] is perfectly fine.

var cellWidth = xFraction * (east - west);
var yFraction = cellSide / (distance(point([west, south]), point([west, north]), options));
var cellHeight = yFraction * (north - south);
var xFraction = cellSide / (distance(point([west, south]), point([east, south]), units));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Distance no longer requires the input data to be Point, it can be an Array of numbers.

distance([west, south], [east, south], options)

Also, all modules have changed to the options param syntax, even the ones with 1 param.

var properties = options.properties || {};
if (!isObject(options)) throw new Error('options is invalid');
var units = options.units;
var maskIsPoly = options.mask && (getType(options.mask) === 'Polygon' || getType(options.mask) === 'MultiPolygon');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use a maskIsPoly approach, instead add some type validation for the input and call it mask.

if (inside(pt, bboxMask)) {
var pt = point([currentX, currentY], properties);
if (maskIsPoly) {
if (inside(pt, options.mask)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all optional parameters, you should use the param name mask instead of options.mask.

@stebogit
Copy link
Collaborator Author

stebogit commented Oct 21, 2017

Thanks @DenisCarriere! I'll work on Mercator friendly outputs 👍

I'm in Boulder at SOTM now.

How's it goin'?
I'd love to attend one of those meetings, somewhere sometime

@DenisCarriere
Copy link
Member

at SOTM now.
@stebogit Definitely awesome talks and great people, you should definitely come to the next US one.

@DenisCarriere
Copy link
Member

I'll work on Mercator friendly outputs

👍 Nice!

@DenisCarriere
Copy link
Member

  • Add translate properties to grid features (options.properties)
  • Add mask to each grid module (options.mask)
  • Use @turf/boolean-inside and/or @turf/boolean-touches for options.mask

@DenisCarriere
Copy link
Member

@stebogit Woot 🎉 First passing commit.

Next thing to implement is the Mercator outputs and then I think we are ready to merge this PR.

@DenisCarriere
Copy link
Member

@stebogit Going to merge this PR, we can do the mercator part on a different PR.

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

Successfully merging this pull request may close these issues.

2 participants