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: Make autodiff deterministic in graph shape #945

Merged
merged 2 commits into from
May 4, 2022
Merged

fix: Make autodiff deterministic in graph shape #945

merged 2 commits into from
May 4, 2022

Conversation

samestep
Copy link
Collaborator

@samestep samestep commented May 3, 2022

Description

This PR fixes the global count hack leftover by #907, replacing it with an ordering on nodes according to breadth-first search and the ordering on edge labels already given by the children function.

Implementation strategy and design decisions

Since this PR follows #939, I used the following commands to update diagrams/ (adapted from .github/workflows/build.yml):

cd packages/automator/
yarn start batch registry.json ../../diagrams/ --src-prefix=../examples/src/

In order to make this work correctly (without --folders), I made a small tweak to packages/automator/index.tsx. I'll update our Registry wiki page after this PR is merged.

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

@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #945 (3405b84) into main (c042fe5) will decrease coverage by 0.00%.
The diff coverage is 78.12%.

@@            Coverage Diff             @@
##             main     #945      +/-   ##
==========================================
- Coverage   63.19%   63.19%   -0.01%     
==========================================
  Files          62       62              
  Lines        7920     7930      +10     
  Branches     1817     1827      +10     
==========================================
+ Hits         5005     5011       +6     
- Misses       2791     2795       +4     
  Partials      124      124              
Impacted Files Coverage Δ
packages/core/src/types/ad.ts 100.00% <ø> (ø)
packages/core/src/engine/Autodiff.ts 79.78% <76.66%> (-0.39%) ⬇️
packages/core/src/engine/AutodiffFunctions.ts 100.00% <100.00%> (ø)

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 c042fe5...3405b84. Read the comment docs.

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 3, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3405b84
Status: ✅  Deploy successful!
Preview URL: https://d662f112.penrose-panes.pages.dev

View logs

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.

Looks great! Glad that the index hack is gone now. Had a small question about idToIndex below. I obviously don't have the context for this. If it's needed, please feel free to just merge this.

packages/core/src/engine/Autodiff.ts Show resolved Hide resolved
@samestep samestep merged commit c6fe4e3 into main May 4, 2022
@samestep samestep deleted the grad-sort branch May 4, 2022 15:33
@samestep
Copy link
Collaborator Author

samestep commented May 4, 2022

I updated the Registry wiki page.

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