feat: Add a function to compute closest points #1039
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #1039 +/- ##
==========================================
+ Coverage 60.81% 61.39% +0.57%
==========================================
Files 62 62
Lines 7582 7693 +111
Branches 1811 1822 +11
==========================================
+ Hits 4611 4723 +112
+ Misses 2870 2869 -1
Partials 101 101
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Deploying with
|
| Latest commit: |
b1ca8e5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d001a1ef.penrose-72l.pages.dev |
| Branch Preview URL: | https://closest-point.penrose-72l.pages.dev |
Merge necessary
…to closest-point
cmumatt
approved these changes
Jun 7, 2022
There was a problem hiding this comment.
This looks good! Suggested some minor improvements, mostly comment-type items. CI wants you to run yarn format in the monorepo root to take care of some formatting. Still, great job on this PR!!
Would also suggest filling in the "Implementation strategy and design decisions" section of the PR. I think you can just explain at a high level that you're calculating Circles one way, Rect-like shapes another, and note any shapes that aren't supported and why.
samestep
changed the title
|
@cmumatt @joshsunshine I cleaned up this PR; do you think it's ready to merge? I haven't yet actually read through all the implementation code for calculating closest points to verify that they're correct. |
± Registry diff📊 PerformanceKeyNote that each bar component rounds up to the nearest 100ms, so each full bar is an overestimate by up to 400ms. Data |
|
One thing I notice is that |
|
Also updating file extensions |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a
closestPointfunction which takes in a shape and a point, and returns the closest point on that shape to the given point.Implementation strategy and design decisions
This PR changes the type of the
clampfunction to acceptad.Nums for all parameters, instead of requiring the bounds of the range to be fixednumbers. The body of the function is the same.Examples with steps to reproduce them
The example in
packages/examples/src/closest-point/plots the closest point to several basic shapes for a single query point (resampling picks new shapes and a new point):Checklist
diagrams/folder