Skip to content

Add find_variants to Extractor#530

Closed
bksaiki wants to merge 4 commits intoegraphs-good:mainfrom
bksaiki:extract-variants
Closed

Add find_variants to Extractor#530
bksaiki wants to merge 4 commits intoegraphs-good:mainfrom
bksaiki:extract-variants

Conversation

@bksaiki
Copy link
Copy Markdown

@bksaiki bksaiki commented Mar 21, 2025

The EGraph type implements extract and extract_variants, but the extractor only implements find_best. Adds find_variants to Extractor which is basically a copy of the original extract_variants function.

@bksaiki bksaiki requested a review from a team as a code owner March 21, 2025 20:53
@bksaiki bksaiki requested review from FTRobbin and removed request for a team March 21, 2025 20:53
@bksaiki
Copy link
Copy Markdown
Author

bksaiki commented Mar 21, 2025

As discussed with @oflatt and @Alex-Fischman. Unsure on the correct return type for Extractor::find_variants since find_best is an Option.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Mar 21, 2025

CodSpeed Performance Report

Merging #530 will not alter performance

Comparing bksaiki:extract-variants (23b5f90) with main (6f49428)

Summary

✅ 10 untouched benchmarks

@Alex-Fischman
Copy link
Copy Markdown
Collaborator

I think that it's fine to not return a Result as long as you document that the *_variants functions might return fewer than limit values (if they don't exist in the e-graph).

Copy link
Copy Markdown
Collaborator

@FTRobbin FTRobbin left a comment

Choose a reason for hiding this comment

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

Looks good to me since it is mostly moving code from egraph to extractor.

But I don't understand the error message extract_variants should be called after extractor initialization. It seems to be an impossible reason unless there is some extractor initialization happening somewhere else.

@Alex-Fischman
Copy link
Copy Markdown
Collaborator

@bksaiki Can you confirm that you don't need this PR merged for the next month-ish? You're just upstreaming a change that herbie-egglog would like?

I would rather wait to merge this until the new backend is in; it seems just as easy to do this in the new backend as it is in this PR, and it will avoid merge conflicts.

@bksaiki
Copy link
Copy Markdown
Author

bksaiki commented Apr 14, 2025

Not high priority

@Alex-Fischman
Copy link
Copy Markdown
Collaborator

The new extractor in #571 will support this. It sounds like Herbie is not using this PR, so are we okay to close? OTOH if you do want it we can merge it before the new backend.

@Alex-Fischman
Copy link
Copy Markdown
Collaborator

Closing after confirming in-person.

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.

3 participants