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: Substance mutations as data + refactored program generator #601

Merged
merged 37 commits into from
Aug 2, 2021
Merged

Conversation

wodeni
Copy link
Member

@wodeni wodeni commented Jun 18, 2021

Description

This PR introduces the Mutation module, which contains all the Substance mutation types and validation functions. The main improvement is that mutations are no longer executed inline in Synthesizer anymore. Instead, mutations are encoded as data and can be executed individually (executeMutation) or in a batch (executeMutations).

Aside from the refactoring, this branch also contains the Search module, which are experimental functions for by-example synthesis. This module is currently unused.

Implementation strategy and design decisions

  • Mutation type: every type of mutation (e.g. Add, Delete, ChangeStmtType etc) has its own attributes enough for executing it. For instance, SwapStmtArgs has the target statement and the indices to swap. A mutation can also have additionalMutations, which are related mutations.
  • Guard functions: there are two ways to construct a mutation: (1) if you have exactly the right parameters in the right types, you can directly construct the object of one of the Mutation types; (2) if you only have a vague target such as SubStmt (which might be something like AutoLabel All and not a valid target for any of the mutations), you can use the check* guard functions to check if a certain kind of mutation is possible for the given target.
  • Callbacks in guard functions: each guard function has a callback function, which normally takes in a validated target and outputs a mutation. When you are calling the guard function to check if you can apply a mutation, you need to also supply a callback for generating the components for the replacement.
  • The program generator: now the Synthesizer class mutates Substance programs by enumerating Add, Delete, and Update mutations and randomly selecting one of them in each step.
  • Refactored generate* functions: instead of building up a Substance program through Synthesizer's internal fields opaquely, these generator functions return one or more Substance statements, which is later converted into Add mutations by Synthesizer.

Examples with steps to reproduce them

TODO

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)

Open questions

  • generate* functions: the SynthesizerContext passed into them is still mutated instead of returned. Should we make them completely pure instead?
    • One known problem with the current approach is: these functions mutate the context when generating mutations that might not selected at the end, so you'll see extra ids even if the mutations are never executed :(.
      • RESOLVED: implemented pure context passing in f68d19e
  • Synthesizer doesn't enumerate all the mutations. Instead, it's still making some random choices such as picking a particular statement to edit.
  • weights is still not used in the system for picking among Add, Delete and Update.

@wodeni wodeni marked this pull request as draft June 18, 2021 20:35
@wodeni wodeni changed the title feat: synthesize Substance programs by example feat: Substance mutations as data + refactored program generator Jul 21, 2021
@codecov
Copy link

codecov bot commented Jul 21, 2021

Codecov Report

Merging #601 (ca3a241) into main (e5796fb) will decrease coverage by 1.02%.
The diff coverage is 39.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #601      +/-   ##
==========================================
- Coverage   64.90%   63.88%   -1.03%     
==========================================
  Files          40       43       +3     
  Lines        6734     7074     +340     
  Branches     1298     1342      +44     
==========================================
+ Hits         4371     4519     +148     
- Misses       2355     2546     +191     
- Partials        8        9       +1     
Impacted Files Coverage Δ
packages/core/src/compiler/Substance.ts 89.71% <6.25%> (-4.53%) ⬇️
packages/core/src/synthesis/Synthesizer.ts 20.76% <18.05%> (-12.19%) ⬇️
packages/core/src/synthesis/Mutation.ts 22.97% <22.97%> (ø)
packages/core/src/index.ts 78.03% <66.66%> (+0.33%) ⬆️
packages/core/src/synthesis/Search.ts 88.33% <88.33%> (ø)
packages/core/src/analysis/SubstanceAnalysis.ts 52.55% <96.22%> (+23.98%) ⬆️
packages/core/src/engine/Autodiff.ts 69.72% <100.00%> (ø)
packages/core/src/types/ast.ts 100.00% <100.00%> (ø)
... and 1 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 e5796fb...ca3a241. Read the comment docs.

@wodeni wodeni marked this pull request as ready for review July 22, 2021 14:29
Copy link
Contributor

@hsharriman hsharriman left a comment

Choose a reason for hiding this comment

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

LGTM! These changes have definitely made the code a lot more modular which is great, and I can see that inserting a new mutation will take only a modest amount of work now. Looking forward to trying it out when I implement the new swap-in mutator! I left some comments--basically all nits--on the PR.

packages/core/src/synthesis/Mutation.ts Outdated Show resolved Hide resolved
packages/core/src/synthesis/Search.test.ts Show resolved Hide resolved
packages/core/src/synthesis/Mutation.ts Show resolved Hide resolved
packages/core/src/synthesis/Synthesizer.ts Outdated Show resolved Hide resolved
packages/core/src/synthesis/Synthesizer.ts Outdated Show resolved Hide resolved
packages/core/src/synthesis/Mutation.ts Show resolved Hide resolved
packages/core/src/synthesis/Synthesizer.ts Outdated Show resolved Hide resolved
packages/core/src/synthesis/Synthesizer.ts Outdated Show resolved Hide resolved
packages/core/src/synthesis/Search.ts Show resolved Hide resolved
Copy link
Contributor

@hsharriman hsharriman left a comment

Choose a reason for hiding this comment

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

LGTM overall. I was unable to run the new examples on my own machine because of import issues from penrose, but I'm not sure if that's an issue with my machine at the moment or not.

packages/core/src/index.ts Show resolved Hide resolved
@wodeni wodeni merged commit da8f9e5 into main Aug 2, 2021
@wodeni wodeni deleted the pbe branch August 2, 2021 17:58
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

2 participants