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

Add quadbinToBoundary utility #16

Merged
merged 8 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
## Quickstart

To install and build `quadbin-js` locally from source:

```bash
# install dependencies
yarn

# build package once
yarn build
```

To run tests, coverage, or a linter, you should execute `yarn build`, and afterward:

```bash
# run tests once
yarn test
```

## Releases

1. Create a new version: `yarn version [ major | minor | patch | prerelease ]`

2. Execute `yarn publish`
8 changes: 7 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,10 @@ Converts quadbin cell into a xyz tile.
function geometryToCells(geometry: GeoJSONGeometry, resolution: bigint): bigint
```

Returns a list of cells covering a GeoJSON geometry at a given resolution
## cellToBoundary

```javascript
function cellToBoundary(quadbin: bigint): GeoJSONGeometry
```

Converts a Quadbin cell identifier into a geographical boundary represented as a polygon
15 changes: 12 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,23 @@
"build:esm": "tsc -p tsconfig/tsconfig.esm.json",
"build:types": "tsc -p tsconfig/tsconfig.types.json",
"build:umd": "webpack --config tsconfig/webpack.config.cjs",
"lint": "npx prettier --check src",
"lint": "prettier --check src",
"test": "yarn lint && yarn test-fast",
"test-fast": "npx ts-node node_modules/tape/bin/tape test/**/*.spec.js"
"test-fast": "ts-node node_modules/tape/bin/tape test/**/*.spec.js",
"prepublishOnly": "yarn build"
},
"browser": {
"jsdom": false
},
"devDependencies": {
"@babel/register": "^7.13.0",
"@types/geojson": "^7946.0.14",
"babel-loader": "^8.0.0",
"babel-preset-minify": "^0.5.0",
"prettier": "^2.4.1",
"tape": "^5.3.0",
"ts-loader": "^9.2.5",
"ts-node": "^10.9.2",
"typescript": "^4.4.4",
"webpack": "^5.52.1",
"webpack-cli": "^4.8.0"
Expand All @@ -54,6 +57,12 @@
"node": ">=14"
},
"dependencies": {
"@mapbox/tile-cover": "3.0.1"
"@mapbox/tile-cover": "3.0.1",
"@math.gl/web-mercator": "^4.1.0"
},
"packageManager": "[email protected]",
"volta": {
"node": "14.21.3",
"yarn": "1.22.22"
}
}
38 changes: 38 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import {tiles} from '@mapbox/tile-cover';
import {worldToLngLat} from '@math.gl/web-mercator';
import type {Polygon} from 'geojson';

const B = [
0x5555555555555555n,
Expand All @@ -13,6 +15,29 @@ const S = [0n, 1n, 2n, 4n, 8n, 16n];
type Quadbin = bigint;
type Tile = {x: number; y: number; z: number};

const TILE_SIZE = 512;

function quadbinToOffset(quadbin: bigint): [number, number, number] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you rename these 3 functions to be cellTo... and export them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you missed this comment

Copy link
Contributor Author

@jmgaya jmgaya Oct 2, 2024

Choose a reason for hiding this comment

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

I kept them as they are because they have this name in deck.gl, and not only that, but they receive a parameter quadbin, so I think we should re-think it a bit before renaming?

Regarding your export suggestion, done!

Copy link
Collaborator

@felixpalmer felixpalmer Oct 2, 2024

Choose a reason for hiding this comment

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

I think if we are going to add functions to the package we shouldn't change the names later. So best to include them as cellToOffset(cell: bigint) etc, i.e. renaming also the parameter. It's not an issue for deck that the name changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I'm afraid of is that we are proposing some changes that may require a major version, and I'm not sure if we should go for them in this PR, just it 🤷

What's your proposal then? Mine is:

  • Stick to Quadbin type everywhere
  • Use Quadbin instead of cell in all the function names
  • Release a major

Copy link
Collaborator

Choose a reason for hiding this comment

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

As stated originally, I want the API to be aligned with quadbin-py. Which it currently is.

  • Any new functions which are in quadbin-py should have the same name
  • Any additional functions, like cellToOffset should follow the conventions already used

I see no value in renaming the existing function names, that would be a breaking change and require a major version and diverge from quadbin-py.

For the new functions we should stick with the conventions, so using cell in function names and quadbin: Quadbin in the parameters. It doesn't matter to the calling code what the name of the parameter is

Copy link
Collaborator

Choose a reason for hiding this comment

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

The API is modeled on https://github.com/uber/h3-js, which means that it is simpler to port code between the spatial indices, which is not the case if the functions are things like quadbinToBoundary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the new functions we should stick with the conventions, so using cell in function names and quadbin: Quadbin in the parameters. It doesn't matter to the calling code what the name of the parameter is

Sounds like a plan, let's go for it 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll say now, every function:

  • Receives a quadbin: Quadbin
  • Uses cell in its name

Could you please re-re-re-review it 😆 ?

const {x, y, z} = cellToTile(quadbin);
const scale = TILE_SIZE / (1 << z);
return [x * scale, TILE_SIZE - y * scale, scale];
}

function quadbinToWorldBounds(quadbin: bigint, coverage: number): [number[], number[]] {
const [xOffset, yOffset, scale] = quadbinToOffset(quadbin);
return [
[xOffset, yOffset],
[xOffset + coverage * scale, yOffset - coverage * scale]
];
}

function getQuadbinPolygon(quadbin: bigint, coverage = 1): number[] {
const [topLeft, bottomRight] = quadbinToWorldBounds(quadbin, coverage);
const [w, n] = worldToLngLat(topLeft);
const [e, s] = worldToLngLat(bottomRight);
return [e, n, e, s, w, s, w, n, e, n];
}

export function hexToBigInt(hex: string): bigint {
return BigInt(`0x${hex}`);
}
Expand Down Expand Up @@ -89,3 +114,16 @@ export function geometryToCells(geometry, resolution: bigint): Quadbin[] {
max_zoom: zoom
}).map(([x, y, z]) => tileToCell({x, y, z}));
}

export function cellToBoundary(cell: bigint): Polygon {
jmgaya marked this conversation as resolved.
Show resolved Hide resolved
const bbox = getQuadbinPolygon(cell);
const boundary = [
[bbox[0], bbox[1]],
[bbox[2], bbox[3]],
[bbox[4], bbox[5]],
[bbox[6], bbox[7]],
[bbox[0], bbox[1]]
];

return {type: 'Polygon', coordinates: [boundary]};
jmgaya marked this conversation as resolved.
Show resolved Hide resolved
}
60 changes: 58 additions & 2 deletions test/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
cellToParent,
geometryToCells,
getResolution,
hexToBigInt
cellToBoundary
} from 'quadbin';

import {tileToQuadkey} from './quadkey-utils.js';
Expand All @@ -16,6 +16,8 @@ const TEST_TILES = [
{x: 1023, y: 2412, z: 23, q: 5291729562728627583n}
];

const ANY_QUADBIN = BigInt(524800);
jmgaya marked this conversation as resolved.
Show resolved Hide resolved

test('Quadbin conversion', async t => {
for (const {x, y, z, q} of TEST_TILES) {
const tile = {x, y, z};
Expand Down Expand Up @@ -51,7 +53,6 @@ test('Quadbin getParent', async t => {
import PointGeometry from './data/PointGeometry.json' assert {type: 'json'};
import MultiPointGeometry from './data/MultiPointGeometry.json' assert {type: 'json'};
import LineStringGeometry from './data/LineStringGeometry.json' assert {type: 'json'};
import MultiLineStringGeometry from './data/MultiLineStringGeometry.json' assert {type: 'json'};
import PolygonGeometry from './data/PolygonGeometry.json' assert {type: 'json'};
import PolygonAntimeridianGeometry from './data/PolygonAntimeridianGeometry.json' assert {type: 'json'};
import MultiPolygonGeometry from './data/MultiPolygonGeometry.json' assert {type: 'json'};
Expand All @@ -78,3 +79,58 @@ test('Quadbin geometryToCells', async t => {
}
t.end();
});

test('Quadbin cellToBoundary', t => {
for (const {quadbin, expectedPolygon} of [
{
quadbin: BigInt(524800),
expectedPolygon: {
type: 'Polygon',
coordinates: [
[
[180, 85.0511287798066],
[180, -85.05112877980659],
[-180, -85.05112877980659],
[-180, 85.0511287798066],
[180, 85.0511287798066]
]
]
}
},
{
quadbin: BigInt(536903670), // Longitude near +180°
expectedPolygon: {
type: 'Polygon',
coordinates: [
[
[180, 85.0511287798066],
[180, -85.05112877980659],
[-180, -85.05112877980659],
[-180, 85.0511287798066],
[180, 85.0511287798066]
]
]
}
},
{
quadbin: BigInt(536870921), // Longitude near -180°
expectedPolygon: {
type: 'Polygon',
coordinates: [
[
[180, 85.0511287798066],
[180, -85.05112877980659],
[-180, -85.05112877980659],
[-180, 85.0511287798066],
[180, 85.0511287798066]
]
]
}
}
]) {
const result = cellToBoundary(quadbin);
t.deepEquals(result, expectedPolygon);
}

t.end();
});
Loading
Loading