Skip to content

Fix stale and incorrect inline docs across the rewriting/saturation packages#24

Merged
jonathanvdc merged 2 commits intomainfrom
copilot/update-inline-docs
Mar 27, 2026
Merged

Fix stale and incorrect inline docs across the rewriting/saturation packages#24
jonathanvdc merged 2 commits intomainfrom
copilot/update-inline-docs

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 26, 2026

Several inline docs had drifted out of sync with the codebase: renamed types, removed APIs, wrong package paths, and incorrect behavioral descriptions.

Stale API references

  • CommandQueueCommandSchedule/CommandScheduleBuilder: CommandQueue no longer exists. Updated all references in Rewrite.scala, Rule.scala, Applier.scala, rewriting/package.scala, and eqsat/package.scala. Example in Rule.scala now shows the real batching pattern via CommandScheduleBuilder.
  • EGraphWithRecordedApplications wrong package: Was referenced as foresight.eqsat.saturation.* in rewriting/package.scala and PortableMatch.scala; actual location is foresight.eqsat.immutable.*.
  • SearchAndApply.withoutCaching/.withCaching: Neither exists. Fixed to SearchAndApply.immutable / .immutableWithCaching in StochasticRuleApplication.scala and rewriting/package.scala.

Incorrect behavioral descriptions

  • Rewrite.apply: Doc claimed "returned without mutating the e-graph" — the method operates on mutable.EGraph and mutates in place. @return also incorrectly said it returned the e-graph instead of Boolean.
  • CommandSchedule.applyImmutable: Had a spurious @param reification with no corresponding parameter.
  • eqsat/package.scala: Claimed all e-graphs are "purely functional"; corrected to acknowledge both the immutable and mutable variants.

Formatting

  • immutable/EGraph.scala: Removed extraneous * * double-asterisk artifacts in the class Scaladoc.

Bug fix (surfaced by doc review)

  • StochasticRuleApplication.apply: The overload accepting a random: Random parameter documented and accepted it but silently discarded it, always constructing with Random(0). Fixed to pass random through to the constructor.

⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.

…GraphWithRecordedApplications package, Rewrite.apply doc, CommandSchedule.applyImmutable spurious param, EGraph.scala formatting, purely-functional claim, StochasticRuleApplication.withoutCaching, and random bug"

Agent-Logs-Url: https://github.com/jonathanvdc/foresight/sessions/b1a9817e-2398-4f10-959c-e417b3ed5e6d

Co-authored-by: jonathanvdc <9839946+jonathanvdc@users.noreply.github.com>
Copilot AI changed the title [WIP] Update inline documentation to sync with repo changes Fix stale and incorrect inline docs across the rewriting/saturation packages Mar 27, 2026
Copilot AI requested a review from jonathanvdc March 27, 2026 00:14
Copy link
Copy Markdown
Owner

@jonathanvdc jonathanvdc left a comment

Choose a reason for hiding this comment

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

Good work so far. I notice that this sentence in eqsat/package.scala is still out of date.

All core data types are persistent values: applying a command, running a rule, or performing extraction yields a new e-graph or analysis result without modifying the original

@github-actions
Copy link
Copy Markdown

JMH comparison (baseline vs PR)

Benchmark Params Baseline PR Δ (PR/Base) Unit
IncrementalBenchmarks.incrementalPolynomial depth=6, mutableEGraph=false, size=500, threadCount=1 199.168 194.836 0.978× (-2.2%) ms/op
IncrementalBenchmarks.incrementalPolynomial depth=6, mutableEGraph=false, size=500, threadCount=2 303.67 298.771 0.984× (-1.6%) ms/op
IncrementalBenchmarks.incrementalPolynomial depth=6, mutableEGraph=true, size=500, threadCount=1 8.45821 8.20938 0.971× (-2.9%) ms/op
IncrementalBenchmarks.incrementalPolynomial depth=6, mutableEGraph=true, size=500, threadCount=2 73.1174 72.4847 0.991× (-0.9%) ms/op
IncrementalBenchmarks.oneByOnePolynomial depth=6, mutableEGraph=false, size=500, threadCount=1 347.445 346.086 0.996× (-0.4%) ms/op
IncrementalBenchmarks.oneByOnePolynomial depth=6, mutableEGraph=false, size=500, threadCount=2 494.228 492.914 0.997× (-0.3%) ms/op
IncrementalBenchmarks.oneByOnePolynomial depth=6, mutableEGraph=true, size=500, threadCount=1 148.835 155.109 1.042× (+4.2%) ms/op
IncrementalBenchmarks.oneByOnePolynomial depth=6, mutableEGraph=true, size=500, threadCount=2 298.189 298.882 1.002× (+0.2%) ms/op
LiarBenchmarks.findGemmInMm threadCount=1 244.289 241.764 0.990× (-1.0%) ms/op
LiarBenchmarks.findGemmInMm threadCount=2 202.146 187.202 0.926× (-7.4%) ms/op
LiarBenchmarks.findGemvInMv threadCount=1 65.7464 63.5807 0.967× (-3.3%) ms/op
LiarBenchmarks.findGemvInMv threadCount=2 62.3901 57.045 0.914× (-8.6%) ms/op
MatmulBenchmarks.nmm mutableEGraph=false, size=20, threadCount=1 4.94789 4.58627 0.927× (-7.3%) ms/op
MatmulBenchmarks.nmm mutableEGraph=false, size=20, threadCount=2 4.49753 4.26286 0.948× (-5.2%) ms/op
MatmulBenchmarks.nmm mutableEGraph=false, size=40, threadCount=1 32.4374 32.0292 0.987× (-1.3%) ms/op
MatmulBenchmarks.nmm mutableEGraph=false, size=40, threadCount=2 25.8516 24.3605 0.942× (-5.8%) ms/op
MatmulBenchmarks.nmm mutableEGraph=false, size=80, threadCount=1 272.757 261.722 0.960× (-4.0%) ms/op
MatmulBenchmarks.nmm mutableEGraph=false, size=80, threadCount=2 181.255 169.317 0.934× (-6.6%) ms/op
MatmulBenchmarks.nmm mutableEGraph=true, size=20, threadCount=1 2.96469 2.96497 1.000× (+0.0%) ms/op
MatmulBenchmarks.nmm mutableEGraph=true, size=20, threadCount=2 2.85135 2.81257 0.986× (-1.4%) ms/op
MatmulBenchmarks.nmm mutableEGraph=true, size=40, threadCount=1 22.6483 22.219 0.981× (-1.9%) ms/op
MatmulBenchmarks.nmm mutableEGraph=true, size=40, threadCount=2 18.5296 19.0589 1.029× (+2.9%) ms/op
MatmulBenchmarks.nmm mutableEGraph=true, size=80, threadCount=1 179.17 173.358 0.968× (-3.2%) ms/op
MatmulBenchmarks.nmm mutableEGraph=true, size=80, threadCount=2 112.906 109.637 0.971× (-2.9%) ms/op
PolyBenchmarks.polynomial mutableEGraph=false, size=5, threadCount=1 96.2169 90.2696 0.938× (-6.2%) ms/op
PolyBenchmarks.polynomial mutableEGraph=false, size=5, threadCount=2 90.6727 83.5585 0.922× (-7.8%) ms/op
PolyBenchmarks.polynomial mutableEGraph=false, size=6, threadCount=1 449.154 419.646 0.934× (-6.6%) ms/op
PolyBenchmarks.polynomial mutableEGraph=false, size=6, threadCount=2 405.41 388.228 0.958× (-4.2%) ms/op
PolyBenchmarks.polynomial mutableEGraph=true, size=5, threadCount=1 37.5006 36.5627 0.975× (-2.5%) ms/op
PolyBenchmarks.polynomial mutableEGraph=true, size=5, threadCount=2 33.4031 31.6633 0.948× (-5.2%) ms/op
PolyBenchmarks.polynomial mutableEGraph=true, size=6, threadCount=1 170.39 153.066 0.898× (-10.2%) ms/op
PolyBenchmarks.polynomial mutableEGraph=true, size=6, threadCount=2 143.478 129.666 0.904× (-9.6%) ms/op
VectorBenchmarks.blinnPhong mutableEGraph=false, threadCount=1 34703.1 31976.6 0.921× (-7.9%) ms/op
VectorBenchmarks.blinnPhong mutableEGraph=false, threadCount=2 32289.8 32865 1.018× (+1.8%) ms/op
VectorBenchmarks.blinnPhong mutableEGraph=true, threadCount=1 5188.05 4867.81 0.938× (-6.2%) ms/op
VectorBenchmarks.blinnPhong mutableEGraph=true, threadCount=2 4187.89 3355.7 0.801× (-19.9%) ms/op
VectorBenchmarks.gramSchmidt mutableEGraph=false, threadCount=1 532.233 500.174 0.940× (-6.0%) ms/op
VectorBenchmarks.gramSchmidt mutableEGraph=false, threadCount=2 434.228 400.194 0.922× (-7.8%) ms/op
VectorBenchmarks.gramSchmidt mutableEGraph=true, threadCount=1 213.297 196.155 0.920× (-8.0%) ms/op
VectorBenchmarks.gramSchmidt mutableEGraph=true, threadCount=2 153.753 155.29 1.010× (+1.0%) ms/op
VectorBenchmarks.reflection mutableEGraph=false, threadCount=1 323.086 299.737 0.928× (-7.2%) ms/op
VectorBenchmarks.reflection mutableEGraph=false, threadCount=2 278.895 249.863 0.896× (-10.4%) ms/op
VectorBenchmarks.reflection mutableEGraph=true, threadCount=1 138.564 127.703 0.922× (-7.8%) ms/op
VectorBenchmarks.reflection mutableEGraph=true, threadCount=2 105.022 92.1852 0.878× (-12.2%) ms/op
VectorBenchmarks.vectorNormalization mutableEGraph=false, threadCount=1 4.04433 3.80589 0.941× (-5.9%) ms/op
VectorBenchmarks.vectorNormalization mutableEGraph=false, threadCount=2 4.4098 4.1523 0.942× (-5.8%) ms/op
VectorBenchmarks.vectorNormalization mutableEGraph=true, threadCount=1 1.91433 1.78719 0.934× (-6.6%) ms/op
VectorBenchmarks.vectorNormalization mutableEGraph=true, threadCount=2 2.36485 2.23058 0.943× (-5.7%) ms/op
Geomean threadCount=1, mutableEGraph=false 0.954× (-4.6%)
Geomean threadCount=1, mutableEGraph=true 0.958× (-4.2%)
Geomean threadCount=2, mutableEGraph=false 0.946× (-5.4%)
Geomean threadCount=2, mutableEGraph=true 0.949× (-5.1%)

Note: < 1.0× means faster on PR; > 1.0× means slower.

@jonathanvdc jonathanvdc merged commit aca0583 into main Mar 27, 2026
2 of 3 checks passed
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.

2 participants