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

fix: multiple matching #1063

Merged
merged 13 commits into from Jul 19, 2022
Merged

fix: multiple matching #1063

merged 13 commits into from Jul 19, 2022

Conversation

liangyiliang
Copy link
Contributor

@liangyiliang liangyiliang commented Jul 14, 2022

Description

Related issue/PR: #1062 AND #1064

This PR fixes #1062 and #1064 using the proposed resolution as written in #1064.

Implementation strategy and design decisions

See comments of #1064

Examples with steps to reproduce them

-- Substance
Hydrogen H1, H2
Oxygen O
Bond( O, H1 )
Bond( O, H2 )

-- Style
canvas {
    width = 50
    height = 40
}

forall Oxygen o; Hydrogen h1; Hydrogen h2
where Bond(o,h1); Bond(o,h2) {
    myText = Text {
        string: "Water!"
        fillColor: rgba(0, 0, 0, 255)
    }
}

-- Domain
type Atom
type Hydrogen <: Atom
type Oxygen <: Atom
predicate Bond( Atom a1, Atom a2 )

The current version prints two "Water!" strings on the canvas, even though the Substance program only has one water molecule. After this fix, it only prints one "Water!" as expected.

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

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jul 14, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: af5da28
Status: ✅  Deploy successful!
Preview URL: https://d7b22c57.penrose-72l.pages.dev
Branch Preview URL: https://multiple-matching.penrose-72l.pages.dev

View logs

@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #1063 (6734c0f) into main (80e0a61) will increase coverage by 0.15%.
The diff coverage is 79.24%.

❗ Current head 6734c0f differs from pull request most recent head af5da28. Consider uploading reports for the commit af5da28 to get more accurate results

@@            Coverage Diff             @@
##             main    #1063      +/-   ##
==========================================
+ Coverage   63.83%   63.98%   +0.15%     
==========================================
  Files          59       59              
  Lines        7273     7309      +36     
  Branches     1629     1637       +8     
==========================================
+ Hits         4643     4677      +34     
- Misses       2528     2623      +95     
+ Partials      102        9      -93     
Impacted Files Coverage Δ
packages/core/src/compiler/Style.ts 66.69% <79.24%> (+0.67%) ⬆️
packages/core/src/utils/Util.ts 58.56% <0.00%> (ø)
packages/core/src/contrib/Utils.ts 32.75% <0.00%> (ø)
packages/core/src/renderer/Line.ts 6.66% <0.00%> (ø)
packages/core/src/renderer/Path.ts 10.12% <0.00%> (ø)
packages/core/src/renderer/Image.ts 12.50% <0.00%> (ø)
packages/core/src/engine/Autodiff.ts 83.68% <0.00%> (ø)
packages/core/src/engine/Optimizer.ts 80.12% <0.00%> (ø)
packages/core/src/renderer/Staging.ts 7.14% <0.00%> (ø)
packages/core/src/utils/toCustomAD.ts 88.27% <0.00%> (ø)
... and 12 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 80e0a61...af5da28. Read the comment docs.

@liangyiliang liangyiliang changed the title fix: multiple matching [WIP] fix: multiple matching Jul 15, 2022
@liangyiliang liangyiliang marked this pull request as ready for review July 15, 2022 18:50
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.

This looks great!

@liangyiliang liangyiliang marked this pull request as draft July 15, 2022 21:18
@liangyiliang
Copy link
Contributor Author

Turns out there are some pending questions that need to be resolved - I'm not merging the code yet.

@liangyiliang liangyiliang linked an issue Jul 15, 2022 that may be closed by this pull request
@liangyiliang liangyiliang marked this pull request as ready for review July 18, 2022 18:39
@samestep
Copy link
Collaborator

For diagrams/non-convex-non-convex.svg, why does it look like a couple of the little triangles either changed shape or merged together?

@samestep
Copy link
Collaborator

For diagrams/small-graph-disjoint-rect-line-horiz.svg, why are a bunch of the rectangles disjoint in the old diagram but not the new one?

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 great! Again I’d like to get @wodeni’s approval on this before merging.

);
const substTargets = im.Set<string>(Object.values(subst));
if (matchedRels !== undefined) {
const record: im.Record<MatchesObject> = im.Record({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, I didn’t know about Record before!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out, im.Set doesn't play nice if I just put JS/TS arrays in the Set.

@liangyiliang
Copy link
Contributor Author

For diagrams/small-graph-disjoint-rect-line-horiz.svg, why are a bunch of the rectangles disjoint in the old diagram but not the new one?

i'm actually not sure, lemme look into that

@liangyiliang
Copy link
Contributor Author

liangyiliang commented Jul 18, 2022

For diagrams/small-graph-disjoint-rect-line-horiz.svg, why are a bunch of the rectangles disjoint in the old diagram but not the new one?

Looks like that is a local minimum - after resampling, they are disjoint.

Update: Fixed in new commit - selected a new random variation.

@liangyiliang
Copy link
Contributor Author

For diagrams/non-convex-non-convex.svg, why does it look like a couple of the little triangles either changed shape or merged together?

They are not merged together - they are still disjoint. The Style block for Triangle has

    -- Try getting close to the center
    encourage equal(0.0, x.a[0])
    encourage equal(0.0, x.a[1])

so they try to clump together. This shows how the objective function changed - it is now assigning less weight to things like Triangle x; Triangle y, and more weight to a single Triangle x!

@liangyiliang liangyiliang merged commit eb0991b into main Jul 19, 2022
@liangyiliang liangyiliang deleted the multiple-matching branch July 21, 2022 20:32
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.

Multiple Matching, continued Multiple Matching Issue
2 participants