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

feat: Minkowski penalties #648

Merged
merged 10 commits into from Jan 4, 2022
Merged

feat: Minkowski penalties #648

merged 10 commits into from Jan 4, 2022

Conversation

jiriminarcik
Copy link
Contributor

@jiriminarcik jiriminarcik commented Sep 16, 2021

Description

This PR reimplements the disjoint function in packages/core/src/contrib/Constraints.ts for rectangles and convex polygons. The disjoint constraint for shapes A and B is implemented as a distance from the origin to the Minkowski sum of A and -B. The mathematical approach is described in more detail here.

Example of optimization for axis-aligned rectangles:

Example of optimization for polygons (custom convex partitioning was used for the static shape):

This implementation also works for 1-simplexes (line segments):

Implementation strategy and design decisions

The current implementation supports convex polygons only. The support for non-convex polygons can be added by implementing a convex partitioning algorithm in the function convexPartitions in packages/core/src/contrib/Constraints.ts.

This approach can lead to large computational graphs, which cause slow code generation step of the autodiff engine (see #652 for more details). The full computational graphs and their properties for two basic interactions are listed below.

Two axis-aligned rectangles (187 nodes, 269 edges and tree depth is 21):
rectangles-graph

Two triangles (866 nodes, 1312 edges and tree depth is 35):
triangles-graph

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing tests pass locally using yarn test
  • I ran yarn docs and there were no errors when generating the HTML site
  • My code follows the style guidelines of this project (e.g.: no ESLint warnings)

@jiriminarcik jiriminarcik changed the title Minkowski penalties feat: Minkowski penalties Sep 16, 2021
@wodeni
Copy link
Member

wodeni commented Sep 28, 2021

@jiriminarcik do you mind pasting in paths to examples from this branch? Would be very helpful if you can point us to where you were having performance problems with the autodiff codegen.

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jan 3, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5730786
Status: ✅  Deploy successful!
Preview URL: https://ffef8d3f.penrose-panes.pages.dev

View logs

@codecov
Copy link

codecov bot commented Jan 3, 2022

Codecov Report

Merging #648 (5730786) into main (847c033) will decrease coverage by 0.16%.
The diff coverage is 54.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #648      +/-   ##
==========================================
- Coverage   65.50%   65.34%   -0.17%     
==========================================
  Files          46       46              
  Lines        7900     7992      +92     
  Branches     1502     1518      +16     
==========================================
+ Hits         5175     5222      +47     
- Misses       2716     2761      +45     
  Partials        9        9              
Impacted Files Coverage Δ
packages/core/src/contrib/Constraints.ts 59.52% <44.68%> (-2.36%) ⬇️
packages/core/src/engine/Autodiff.ts 59.22% <64.00%> (+0.20%) ⬆️
packages/core/src/engine/BBox.ts 92.59% <0.00%> (-3.71%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 847c033...5730786. Read the comment docs.

Copy link
Member

@wodeni wodeni left a comment

Choose a reason for hiding this comment

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

Overall looks great to me! I haven't looked at the theory. It would be good if someone (maybe @keenancrane?) can have a look at the math here. Just two comments on AD/perf below.

add(
varOf(10e-15),
add(
squared(max(sub(xp, xr), varOf(0.0))),
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% certain on this: should constOf be used here? Looking at the impl of them, seems like the only difference is constOf names the node differently (

return variableAD(x, String(x), "const");
). I wonder if there's any performance implications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can see, there is no difference in the functionality.. And it seems to be used interchangeably across the codebase. But I'll change it to constOf everywhere I have an explicit number inside.

// Compute coordinates of the new rectangle
const xs = [sub(xa1, xb1), sub(xa2, xb2), sub(xa1, xb2), sub(xa2, xb1)];
const ys = [sub(ya1, yb1), sub(ya2, yb2), sub(ya1, yb2), sub(ya2, yb1)];
const xc1 = xs.reduce((x, y) => min(x, y, true));
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the reduce...min operation is quite common. Perhaps it makes sense to write potentially more performant minimum and maximum functions that directly operate on VarAD[]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added minN and maxN. This also led to some readability improvements.

@keenancrane
Copy link
Collaborator

Looks great to me!

The PDF has a reference to [4], which seems to be missing. (But probably fine to go ahead with the PR…)

@jiriminarcik
Copy link
Contributor Author

Looks great to me!

The PDF has a reference to [4], which seems to be missing. (But probably fine to go ahead with the PR…)

fixed

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.

None yet

4 participants