Updating circle, ellipse, line-arc, and sector to use smarter approximations

[lemmingapex]

Summary

See the details and linked post in #2897

The focus of this work is to update the turf.circle(...) to use a smarter regular polygon approximation. Related packages have also been updated to use more accurate approximations including ellipse, line-arc, and sector. These packages should be as consistent with each other as they are in version 7.2.0.

Please let me know if you find any issues or have any questions, I would be happy to assist if I can. Thanks for your help.

Tickets

closes #2897

Tests and Docs

All related tests and documentation have been updated.

Changes

The returned construction for all mentioned packages above will now differ from the current turf version (7.2.0). A new option, options.maximumRimDeviation, is now available for some of the mentioned packages.

Other

• Confirmation you've read the steps for preparing a pull request.

[lemmingapex]

Hey @smallsaucepan. Thanks for your help with this. Any estimation on when this might be merged and when the next release of turf is scheduled for? Let me know what you need.

[lemmingapex]

Looking to get an update on this. Any information @smallsaucepan can provide would be helpful. Thanks!

[lemmingapex]

Hey @mfedderly, I'm looking to get an update on the status of this MR. Can you give an estimate on when this might be merged and released?

[mfedderly]

👋 I was looking at this PR a few days ago. The writeup is really excellent.

I've been trying to think about whether the implementation detail of being an inscribed polygon (and thus none of the points ever fall outside the actual circle) is part of the public API contract or not.

I left some more thoughts inline.

[mfedderly]

These feel too internal to be exposed through @turf/helpers, but so far we haven't built anything to share code across packages and keep it all in sync. I'm not sure if I want to try some sort of code duplication mechanism in the monorepo or start adding very small support packages for code like this so that it will still be deduplicated in bundlers.

I think my first instinct would be to make some @turf/internal-* packages which we don't publish API docs for, but we treat as public API from a versioning standpoint. I still don't like it, but I think that's a better outcome than trying to share code and having it wind up duplicated across packages, even if the implementation itself isn't very large. I considered simlinks, but that sounds like it requires some config on Windows to make Git work, and another config in TypeScript to make that work. I don't really want to add a file-copy step to the repo initialization on a fresh checkout.

[lemmingapex]

I agree about the visibility of these methods. I wasn't sure where else they should go with the current architecture. Whatever you decide here is good with me, just let me know how you want to proceed and where an appropriate place for this code might be.

[mfedderly]

Its not clear what units the rim deviation is measured in, and having this just override the steps argument seems like a bit of a force to maintain back compatability.

I'm trying to release as many fixes from the PR backlog as 7.4 in the near term, but then I'm going to move to work on v8 and merge a bunch of the breaking changes that have been sitting around. Would you make this interface differently if you could make a breaking change?

I'm on the fence on whether I want to add options.method (one of "inscribed" | "circumscribed" | "equiareal"), or just yoink everyone onto the newer more accurate version. Even in the world where we just have this only this implementation, there may still be a desire to bound the number of steps so you don't wind up with a polygon with way more points than expected and something downstream becomes less performant. So steps and maximumRimDeviation may both be here to stay in some form?

There's a similar break I'm going to be making in @turf/buffer when I move us to the new clipper2 implementation in v8. It currently uses steps but clipper2 under the hood exposes arcTolerance instead.

[lemmingapex]

maximumRimDeviation should have the same units as radius. Do you think it makes sense to have a comment or note about the units in the @param to make this clear?

Regarding the options.method, we could certainly make this happen alongside steps and maximumRimDeviation. This has pros and cons. It gives the caller the power to pick the kind of circle they want, but also introduces some complexity, especially if we expose this method option on the related methods/functions like ellipse, line-arc, and sector.

Regarding the steps and maximumRimDeviation parameters, I think a discriminated union using TypeScript would make steps and maximumRimDeviation mutually exclusive. The caller can provide one or the other, but not both: preventing an invalid state entirely. It would look something like this:

/** Precision controlled by fixed step count */
type StepsPrecision = {
  /** Number of steps/sides for the generated polygon (default: 64) */
  steps?: number;
  maximumRimDeviation?: never;
};

/** Precision controlled by maximum deviation/tolerance threshold */
type DeviationPrecision = {
  steps?: never;
  /** Maximum allowable distance error between the polygon rim and the true circle */
  maximumRimDeviation: number;
};

/** Optional configuration for generating circle polygons */
export type CircleOptions<P extends GeoJsonProperties = GeoJsonProperties> = {
  /** Turf distance unit for radius and tolerance calculations */
  units?: Units;
  /** Custom GeoJSON properties to attach to the resulting Feature */
  properties?: P;
} & (StepsPrecision | DeviationPrecision);

Please let me know what you would like to do.

[mfedderly]

I'd have to do a quick benchmark to reason about any performance changes here. Since its just in the initialization I expect this to be mostly in the same ballpark speed wise.

[lemmingapex]

calculatePolygonCircumRadiusToBestApproximateEqualAreaCircle is fast. It executes in constant time $\mathcal{O}(1)$ and runs in under 10–20 nanoseconds per call in modern JS engines. I can make one or two micro-optimizations that may shave off 1 or 2 nanoseconds potentially. I didn't notice any timing changes in the current tests with this addition. Does this make sense?

[mfedderly]

Yeah I'm not suggesting optimizing now, but before merging I just want to make sure we don't introduce some large performance regression that anyone will actually notice. We transitioned union and a few other packages to polyclip-ts for correctness, but it uses BigDecimal underneath for that correctness which turned out to be very impactful performance wise. So its something I try to keep an eye on as we make changes.

(I'm willing to introduce performance hits when it meaningfully impacts correctness, but I try to keep the impact to a minimum where possible)

[lemmingapex]

This makes perfect sense! I agree with that.

[lemmingapex]

I have optimized the calculatePolygonCircumRadiusToBestApproximateEqualAreaCircle code. This method has a duration in the single digit nanosecond range at this point. There's only one other optimization I can think of, which would be to precompute and store a scaling factor for the default number of steps. This type of optimization is getting into sub-nanosecond territory.

[mfedderly]

A quick look here makes me think that its constructing twice as many Position[] in the new implementation? So that might have a bigger hit to the performance than I'd be comfortable with.

Open to being wrong here I didn't pull the branch and bench it against master

[lemmingapex]

Yeah, I think you are correct here about there now being two calls to constructCoords, rather than one. I'm happy to try and optimize this construction or dig deeper into the fundamentals of the ellipse if it's decided that performance has degraded. Let me know what you think is best here!

[lemmingapex]

Thanks for your feedback @mfedderly! I'm happy to work with you to get this MR into a good spot. I will address all of the inline comments best I can, but I defer to your expertise on what you think is best regarding defaults, apis and performance, etc. moving forward. Thanks for your help!

[mfedderly]

Sounds good. I'm going to try to get 7.4.0 out into the world sooner rather than later and then I'll circle back to this before heading down the 8.0 path just in case we want it in 7.x before the rest of the breaks. 7.4.0 is going to be a big enough release already anyhow since I've merged so much in the past few weeks.

[lemmingapex]

I think I will rename maximumRimDeviation to tolerance in these api changes. maximumRimDeviation is descriptive, but it's quite verbose. tolerance aligns with established domain jargon (GDAL, Leaflet, Turf, PostGIS, etc.). What do you think @mfedderly?