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

interactive: add functions that make xdsl-gui add rewrite functionality to tool (4/4) #2131

Closed
wants to merge 45 commits into from

Conversation

dshaaban01
Copy link
Collaborator

PR 4 out of 4. Broke the last one, had to re-pr.

In this final PR, we add functions that compute the list of "current_possible_rewrites" (i.e. rewrites that apply to our input text). these rewrites are added to the "list view", and we adapt the function to correctly update the pass_pipeline if a rewrite is selected to applied in a way that leverages the "Individual Rewrite" pass that we created previously.

We have thus been able to reuse the pass infrastructure for rewrites.

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (7972c6b) 90.18% compared to head (a65424c) 90.19%.

Files Patch % Lines
xdsl/interactive/app.py 84.84% 5 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                    @@
##           dalia/interactive/oopsie    #2131   +/-   ##
=========================================================
  Coverage                     90.18%   90.19%           
=========================================================
  Files                           307      307           
  Lines                         37090    37126   +36     
  Branches                       5521     5524    +3     
=========================================================
+ Hits                          33451    33487   +36     
- Misses                         2860     2861    +1     
+ Partials                        779      778    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dshaaban01 dshaaban01 changed the title interactive: add functions that make xdsl-gui add rewrite functionality to tool interactive: add functions that make xdsl-gui add rewrite functionality to tool (4/4) Feb 8, 2024
Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

Really nice!
I want to say though, we should change the way we test this at some point.
Right now, we are testing that the condense button will return a set of passes that we are expecting. But once people are adding new passes, or new rewrites, they will need to change these tests as well.
I don't think that's good design, so I'm wondering how else should we do this (in another PR).

Probably, we should check that the resulting passes all have an effect, and that the passes that were not added had no effects.
But to be honest, maybe we don't really have to test this at all, because that might be more annoying than not to test the condense button.

@dshaaban01
Copy link
Collaborator Author

Really nice! I want to say though, we should change the way we test this at some point. Right now, we are testing that the condense button will return a set of passes that we are expecting. But once people are adding new passes, or new rewrites, they will need to change these tests as well. I don't think that's good design, so I'm wondering how else should we do this (in another PR).

Probably, we should check that the resulting passes all have an effect, and that the passes that were not added had no effects. But to be honest, maybe we don't really have to test this at all, because that might be more annoying than not to test the condense button.

we can discuss it in our next weekly

@webmiche
Copy link
Collaborator

Probably, we should check that the resulting passes all have an effect, and that the passes that were not added had no effects. But to be honest, maybe we don't really have to test this at all, because that might be more annoying than not to test the condense button.

Or instead of doing this for all passes in xDSL, we implement two trivial passes, one with changes, one without, and check that it condenses the list of those two properly.

Base automatically changed from dalia/interactive/oopsie to main February 13, 2024 07:52
@dshaaban01 dshaaban01 closed this Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interactive xdsl-gui things tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants