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

🛠 Extend Circle Options #2516

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

AminoffZ
Copy link
Contributor

@AminoffZ AminoffZ commented Oct 19, 2023

id and bbox options for @turf/circle

Functions like polygon, which circle depends on, has options to directly assign id or bbox. This change makes it possible to set these using additional parameters.

Changes

  • Extend options parameter in the circle function to allow setting bbox and id directly.
  • Change properties: any to properties: P | undefined
  • Define const stepAngle = -360 / steps; outside of the loop.

Test and Bench

Test

Didn't have time to write more tests for this case, can do if the idea is approved 👍.

TAP version 13
#turf-circle
ok 1 circle1
#turf-circle -- validate geojson

1..1
#tests 1
#pass 1

#ok

Bench

Before:

turf-circle - 16 steps x 61,962 ops/sec ±1.17% (82 runs sampled)
turf-circle - 32 steps x 58,222 ops/sec ±2.00% (83 runs sampled)
turf-circle - 64 steps x 62,992 ops/sec ±0.35% (91 runs sampled)

After:

turf-circle - 16 steps x 65,058 ops/sec ±2.12% (82 runs sampled)
turf-circle - 32 steps x 61,139 ops/sec ±1.13% (78 runs sampled)
turf-circle - 64 steps x 62,216 ops/sec ±0.62% (94 runs sampled)


Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.
    • This errors though I have formatted, help 😐:
~/turf$ npm run lint

> lint
> npm-run-all lint:*


> lint:eslint
> eslint packages


> lint:prettier
> prettier --check .

Checking formatting...
[warn] packages/turf-circle/index.ts
[warn] Code style issues found in the above file(s). Forgot to run Prettier?
ERROR: "lint:prettier" exited with 1.

@AminoffZ AminoffZ changed the title 🛠 Add Circle Options 🛠 Extend Circle Options Oct 19, 2023
Copy link
Collaborator

@twelch twelch left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution @AminoffZ. I'd like to request a few changes:

  • You seem to have included a number of file permission changes as part of this PR. Can you please remove these?

Screenshot:
image

  • You offered to added a couple more tests for this functionality, those would be welcome.
  • Check the CI test result for any issues.
  • You can run prettier manually using a command like npx prettier --write .. Or possibly install prettier to be run automatically by your editor of choice.

See the rest of my comments in the code.


if (!Array.isArray(center) && center.type === "Feature") {
properties = properties || center.properties;
bboxValue = center.bbox;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it makes sense to pass on the bbox of the center point to polygon()? I would suspect that the bbox of the point will almost never be representative of the bbox of the resulting circle. I think it would be better to leave it undefined if the caller doesn't provide their own options.bbox. And the user can generate their own using bbox()


// main
let properties: P | undefined = options.properties;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not essential but consider letting the types flow through without manual duplication like

let properties: Pick<typeof options, 'properties'> = options.properties;

Comment on lines 15 to 16
* @param {Array<number>} [options.bbox] Bounding Box Array [west, south, east, north] associated with the Feature
* @param {string|number} [options.id] Identifier associated with the Feature
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can these please clarify these relate to the result Feature, for example "Identifier to assign to the resulting circle feature", "Bounding Box Array [west, south, east, north] to assign to the resulting circle Feature"

Copy link
Collaborator

@twelch twelch Nov 14, 2023

Choose a reason for hiding this comment

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

Can the center param typedoc also please clarify that if it is a Feature, then its properties/id will be assigned to the resulting circle feature, and whether they will have precedence over options passed.

Comment on lines 55 to 61
let _options: { bbox?: BBox; id?: Id } | undefined;
if (bboxValue || idValue) {
_options = {
...(bboxValue ? { bbox: bboxValue } : {}),
...(idValue ? { id: idValue } : {}),
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels a bit overcomplicated. Can it be removed altogether and you can construct the options object in the call to polygon()? For example:

return polygon([coordinates], properties, {bbox: bboxValue, id: idValue);

@AminoffZ
Copy link
Contributor Author

Thank you for your review and suggestion @twelch! I'm going to have a pretty busy coming few days but will get back to this when I have the time 🙂, ETA ~2 weeks.

@AminoffZ
Copy link
Contributor Author

ok lol, i have some git setting or something that keeps messing up the file permissions (i'm on wsl2 ubuntu)... will look into it later. i added some of your suggestions, haven't gotten around to the tests yet. i think that will take quite a while to research.

on another note:
turf-area/bench does currently not appear to use the circle function correctly
.add("turf-circle - 16 steps", () => circle(center, radius, 16))
when it should be
.add("turf-circle - 16 steps", () => circle(center, radius, { steps: 16 }))
these could be changed accordingly as well as new benchmark results.

finally #2248 which probably was due to a different issue at time of posting, is now relevant again.

Argument of type '{ shallow: true; }' is not assignable to parameter of type '{ external: string[]; shallow?: boolean | undefined; order?: any[] | undefined; access?: string[] | undefined; hljs?: { highlightAuto?: boolean | undefined; languages?: any[] | undefined; } | undefined; inferPrivate?: string | undefined; extension?: string | ... 1 more ... | undefined; }'.
Property 'external' is missing in type '{ shallow: true; }' but required in type '{ external: string[]; shallow?: boolean | undefined; order?: any[] | undefined; access?: string[] | undefined; hljs?: { highlightAuto?: boolean | undefined; languages?: any[] | undefined; } | undefined; inferPrivate?: string | undefined; extension?: string | ... 1 more ... | undefined; }'.ts(2345)
index.js(193, 4): 'external' is declared here.

on
.build(indexPath, { shallow: true })

results in

git commit -m "Cleanup turf-circle"
husky > pre-commit (node v20.9.0)
✔ Preparing...
⚠ Running tasks...
↓ No staged files match package.json [SKIPPED]
✔ Running tasks for **/.{js,ts}
❯ Running tasks for packages/
/index.{js,ts}
✖ ./scripts/generate-readmes [ENOENT]
✔ Running tasks for */
↓ Skipped because of errors from tasks. [SKIPPED]
✔ Reverting to original state because of errors...
✔ Cleaning up...
✖ ./scripts/generate-readmes failed without output (ENOENT).
husky > pre-commit hook failed (add --no-verify to bypass)

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

Successfully merging this pull request may close these issues.

2 participants