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

Add Product expression and normalising rules #563

Merged
merged 6 commits into from
Jan 7, 2025
Merged

Conversation

niklasdewally
Copy link
Collaborator

Based on: #561
Tracking issue: #505


This PR adds product expressions, and some normalising rules for them.

Changelog

  • ast: add Product expression

  • feat(rules): add canonical ordering for products

    Add reorder_product rule for products, giving them a (partial) canonical ordering.

    Having all products be constant*variable will be useful in detecting weighted sums, which are their own separate constraint in Minion.

  • feat(rules): add collect_like_terms rule for weighted sums

    A weighted sum is a sum of products a*x + b*y + ... where a,b are constants, and x,y variable references. This rule simplifies these sums by combining factors of the same variable together. For example, a*x + ... + b*x ~> (a+b)*x

  • test: add product normalising tests

@niklasdewally niklasdewally added area::conjure-oxide/ast Related to conjure_core and ast representation. area::rules Related to rewrite rules labels Dec 31, 2024
@niklasdewally niklasdewally self-assigned this Dec 31, 2024
@niklasdewally niklasdewally force-pushed the nik/pr/add-mul/01 branch 3 times, most recently from b183e41 to b69781b Compare December 31, 2024 16:33
@niklasdewally niklasdewally marked this pull request as draft January 2, 2025 11:04
@niklasdewally niklasdewally force-pushed the nik/pr/tester-add-run-solver-false/02 branch from 76615b4 to 889cda9 Compare January 3, 2025 21:23
Add `run_solver` config value to integration tests `config.toml` files.

The main motivation for this change is to allow language feature
implementation to be split into multiple PR's and still be testable.
@niklasdewally niklasdewally force-pushed the nik/pr/tester-add-run-solver-false/02 branch from 889cda9 to 0ccef35 Compare January 3, 2025 21:24
@niklasdewally niklasdewally force-pushed the nik/pr/add-mul/01 branch 2 times, most recently from eda3523 to b2248a7 Compare January 3, 2025 21:34
@niklasdewally niklasdewally linked an issue Jan 3, 2025 that may be closed by this pull request
2 tasks
@niklasdewally niklasdewally marked this pull request as ready for review January 3, 2025 21:55
@niklasdewally
Copy link
Collaborator Author

I've rebased this on top of the flat Minion constraints refactor (#566)

Add `reorder_product` rule for products, giving them a (partial)
canonical ordering.

Having all products be constant*variable will be useful in detecting
weighted sums, which are their own separate constraint in Minion.
A weighted sum is a sum of products `a*x + b*y + ...` where a,b are
constants, and x,y variable references. This rule simplifies these sums
by combining factors of the same variable together. For example, `a*x +
... + b*x ~> (a+b)*x`
Increase priority of `normalise_ac` to be above `reorder_product`.

For example weighted-sum/02-needs-normalising does the following:

```
Product([Product([y, 3]), 1]),
   ~~> reorder_product ([("Base", 8800)])
Product([1, Product([y, 3])])

--

Product([y, 3]),
   ~~> reorder_product ([("Base", 8800)])
Product([3, y])

--

Product([y, -5]),
   ~~> reorder_product ([("Base", 8800)])
Product([-5, y])

--

Sum([Sum([Sum([Sum([Product([5, x]), Product([3, Product([1, Product([3, y])])])]), -(Product([3, x]))]), Product([-1, y])]), Product([-5, y])]),
   ~~> normalise_associative_commutative ([("Base", 8400)])
Sum([Product([5, x]), Product([3, Product([1, Product([3, y])])]), -(Product([3, x])), Product([-1, y]), Product([-5, y])])
```

Instead of calling `reorder_product` for each nested product
individually, we should flatten the product then call reorder_product
once on the entire expression.

Our other rules also implicitly assume a flat expression, as it is
simpler, so having this ran at a higher priority makes sense.
Base automatically changed from nik/pr/tester-add-run-solver-false/02 to main January 6, 2025 12:01
@ozgurakgun ozgurakgun merged commit 09ea513 into main Jan 7, 2025
14 checks passed
@niklasdewally niklasdewally deleted the nik/pr/add-mul/01 branch January 7, 2025 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area::conjure-oxide/ast Related to conjure_core and ast representation. area::rules Related to rewrite rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add normalising rule ax + bx ~> (a+b)x (sum_collect_like_terms)
2 participants