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: call evalShapes only twice to generate a computation graph #976

Merged
merged 17 commits into from May 17, 2022

Conversation

wodeni
Copy link
Member

@wodeni wodeni commented May 16, 2022

Description

Related issue/PR: #971

Currently, the system calls shapeAutodiffToNumber(evalShapes(state)) (1) after taking a step in the optimizer, (2) after resampling, and (3) when generating the initial shape. Internally, evalShapes constructs a computational graph with no inputs (specifically, the varying values are plugged in as constants) and just extract the shape properties at the end.

Because the computation graph doesn't change during optimization, there's actually no point re-constructing the graph when evaluating shapes. Instead, it would be much more efficient to compile the computation graph only once, and call the compiled function with new varying values to get the new shapes.

As a next step of #971, in this PR, we call the evaluator only once during compilation, and use a computation graph for evaluation at runtime.

Implementation strategy and design decisions

  • We kept most of Evaluator intact, and use it to generate a comp graph in compileCompGraph.
  • compileCompGraph is called (1) before collectLabels (to refactor later because it really doesn't need fully evaluated shapes), (2) in genOptProblem for actually generating computeShapes (the compiled autodiff function), and (3) after resampling.
  • Because we don't have derivative nodes as secondary outputs (see refactor: Style compiler cleanup #971), derivative and derivativePreconditioned will not be updated during evaluation. Therefore, we decided to stop supporting them.

Examples with steps to reproduce them

See https://github.com/penrose/penrose/suites/6548051374/artifacts/244548940

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new ESLint warnings
  • I have reviewed any generated changes to the diagrams/ folder

Open questions

Questions that require more discussion or to be addressed in future development:

@wodeni wodeni marked this pull request as draft May 16, 2022 16:14
@codecov
Copy link

codecov bot commented May 16, 2022

Codecov Report

Merging #976 (cd0f94b) into main (012aec1) will increase coverage by 0.57%.
The diff coverage is 84.47%.

❗ Current head cd0f94b differs from pull request most recent head 4e02dda. Consider uploading reports for the commit 4e02dda to get more accurate results

@@            Coverage Diff             @@
##             main     #976      +/-   ##
==========================================
+ Coverage   63.13%   63.71%   +0.57%     
==========================================
  Files          62       62              
  Lines        7813     7789      -24     
  Branches     1814     1782      -32     
==========================================
+ Hits         4933     4963      +30     
+ Misses       2762     2714      -48     
+ Partials      118      112       -6     
Impacted Files Coverage Δ
packages/core/src/contrib/Functions.ts 14.43% <ø> (+0.82%) ⬆️
packages/core/src/shapes/Samplers.ts 91.17% <ø> (-0.93%) ⬇️
packages/core/src/engine/Evaluator.ts 31.33% <65.27%> (-2.18%) ⬇️
packages/core/src/compiler/Style.ts 78.20% <100.00%> (-0.09%) ⬇️
packages/core/src/contrib/Constraints.ts 82.11% <100.00%> (+0.60%) ⬆️
packages/core/src/contrib/ConstraintsUtils.ts 82.25% <100.00%> (+1.06%) ⬆️
packages/core/src/contrib/Minkowski.ts 99.20% <100.00%> (+0.36%) ⬆️
...e/src/contrib/__testfixtures__/TestShapes.input.ts 100.00% <100.00%> (ø)
packages/core/src/engine/EngineUtils.ts 65.35% <100.00%> (+2.59%) ⬆️
packages/core/src/engine/Optimizer.ts 81.03% <100.00%> (+0.05%) ⬆️
... and 4 more

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 012aec1...4e02dda. Read the comment docs.

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 16, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 74695d4
Status: ✅  Deploy successful!
Preview URL: https://15a23f5e.penrose-panes.pages.dev

View logs

@samestep samestep changed the title feat: call evalShapes only once to generate a computation graph feat: call evalShapes only twice to generate a computation graph May 17, 2022
Copy link
Collaborator

@samestep samestep left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@wodeni wodeni marked this pull request as ready for review May 17, 2022 21:47
@wodeni wodeni merged commit 0ff28e5 into main May 17, 2022
@wodeni wodeni deleted the eval-graph branch May 17, 2022 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants