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: repel objective on segments causes slow optimization time #613

Merged
merged 7 commits into from Jul 13, 2021

Conversation

hsharriman
Copy link
Contributor

@hsharriman hsharriman commented Jun 25, 2021

Description

Using repel to ensure that text labels are a certain distance from a segment causes optimization times of between 10-15 seconds. Based on the discussion in related issue #607, implemented 2 changes:

  • Added a constraint closestPtToSeg that ensures a given point is a certain distance away from the nearest point on a given segment. As @hypotext mentions in the issue, this is faster because it does not involve sampling the distance at multiple points of the segment. After implementing this objective, the resample time drops from 10-15 seconds to 2-6 seconds.
  • Per @joshpoll suggestions, added a constraint nonDegenerateAngle to push angles to be within a reasonable range (avoid 0 and 180-degree angles). At the moment, this is implemented with a cutoff, so any angles between 10 and 170 degrees have automatically satisfied this objective.

Examples with steps to reproduce them

  1. yarn start
  2. Run npx roger watch packages/synthesizer/__tests__/congruent.sub packages/synthesizer/__tests__/euclidean.sty packages/synthesizer/__tests__/geometry.dsl from penrose root.

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

This was my first time writing a new constraint, and though everything appears to be working properly I'd definitely appreciate feedback on the implementation.
Questions that require more discussion or to be addressed in future development:

@codecov
Copy link

codecov bot commented Jun 25, 2021

Codecov Report

Merging #613 (0a656f7) into main (ec12a72) will decrease coverage by 0.17%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #613      +/-   ##
==========================================
- Coverage   65.56%   65.38%   -0.18%     
==========================================
  Files          39       39              
  Lines        6615     6633      +18     
  Branches     1274     1278       +4     
==========================================
  Hits         4337     4337              
- Misses       2272     2290      +18     
  Partials        6        6              
Impacted Files Coverage Δ
packages/core/src/contrib/Constraints.ts 51.49% <0.00%> (-2.42%) ⬇️

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 ec12a72...0a656f7. Read the comment docs.

packages/core/src/contrib/Constraints.ts Outdated Show resolved Hide resolved
examples/geometry-domain/euclidean-simple.sty Outdated Show resolved Hide resolved
packages/synthesizer/__tests__/euclidean.sty Outdated Show resolved Hide resolved
packages/core/src/contrib/Constraints.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@kai-qu kai-qu left a comment

Choose a reason for hiding this comment

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

These changes look fine to me. @joshpoll @wodeni What do you think, is this ready to merge?

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.

Looking at the code, the changes look good to me. Seems like the perf problem is still there (?), but I think it's good to merge as long as @hypotext and @joshpoll think these functions are better alternatives to repel

packages/core/src/contrib/Constraints.ts Outdated Show resolved Hide resolved
@hsharriman hsharriman requested a review from joshpoll July 6, 2021 18:08
Copy link
Contributor

@joshpoll joshpoll left a comment

Choose a reason for hiding this comment

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

overall lgtm once this change is incorporated I will approve!

packages/core/src/contrib/Constraints.ts Outdated Show resolved Hide resolved
@hsharriman hsharriman requested a review from joshpoll July 7, 2021 17:13
@kai-qu kai-qu merged commit cfc8f46 into main Jul 13, 2021
@kai-qu kai-qu deleted the repel-segment-fix branch July 13, 2021 21:06
@kai-qu
Copy link
Contributor

kai-qu commented Jul 13, 2021

Forgot @joshpoll didn't have merge permission. Done now, thanks @hsharriman!

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