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

Convert Products to Minion #570

Merged
merged 3 commits into from
Jan 8, 2025
Merged

Conversation

niklasdewally
Copy link
Collaborator

@niklasdewally niklasdewally commented Jan 7, 2025

This PR converts Product expressions to Minion. Continuation of #563, which added the Product expression and its normalising rules.

  • rules/minion: flatten Leq,Geq,Lt,Gt using flatten_binop
  • rules: add remove_unit_vector_product
  • Add FlatProductEq and Minion conversion rules

@niklasdewally niklasdewally self-assigned this Jan 7, 2025
@niklasdewally niklasdewally changed the title rules: add remove_unit_vector_product Convert Products to Minion Jan 7, 2025
@niklasdewally niklasdewally marked this pull request as draft January 7, 2025 20:17
@niklasdewally niklasdewally force-pushed the nik/pr/add-mul/minion-conversion branch from b031a3f to d863d73 Compare January 7, 2025 20:39
@niklasdewally niklasdewally marked this pull request as ready for review January 7, 2025 20:43
@niklasdewally niklasdewally force-pushed the nik/pr/add-mul/minion-conversion branch from d863d73 to 08a6f04 Compare January 7, 2025 20:46
Base automatically changed from nik/pr/print-top-level-exprs to main January 8, 2025 12:21
@niklasdewally niklasdewally force-pushed the nik/pr/add-mul/minion-conversion branch from 08a6f04 to ebecc6b Compare January 8, 2025 12:23
Copy link
Contributor

@ozgurakgun ozgurakgun 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 though I think you'll have to rebase due to the changes I made in #568

@niklasdewally niklasdewally force-pushed the nik/pr/add-mul/minion-conversion branch from ebecc6b to 0206838 Compare January 8, 2025 12:31
@niklasdewally niklasdewally merged commit 07b27c4 into main Jan 8, 2025
5 of 14 checks passed
@niklasdewally niklasdewally deleted the nik/pr/add-mul/minion-conversion branch January 8, 2025 12:32
@ozgurakgun
Copy link
Contributor

better if we wait until all tests pass before merging...

@niklasdewally
Copy link
Collaborator Author

better if we wait until all tests pass before merging...

apologies - sausage fingers!

@ozgurakgun
Copy link
Contributor

I enabled auto-deletion, no arguments against this.

I am also happy to configure auto-merge once reviewed&approved + all tests pass. Not sure how to do this however.

Also, I am not sure what the benefit is. It takes away some flexibility in the order of merging.

@niklasdewally
Copy link
Collaborator Author

niklasdewally commented Jan 8, 2025

I enabled auto-deletion, no arguments against this.

Thanks

I am also happy to configure auto-merge once reviewed&approved + all tests pass. Not sure how to do this however.

Also, I am not sure what the benefit is. It takes away some flexibility in the order of merging.

It is not fully automatic: what it does is give you a button to "enable auto merge" on a specific PR, and then when checks pass on that PR, it will merge itself into main.

My minor annoyance is having to keep checking Github to see if my PR has finished its checks yet before merging it, or accidentally clicking merge too early.

From the docs, it looks like we need the "require checks to pass" branch restriction. As repo admin, it looks like you can override these for a PR if needed, so it mainly affects me

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