-
Notifications
You must be signed in to change notification settings - Fork 362
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
Replace BTree with Vec in LinComb #322
Conversation
Release 0.4.4
Awesome, let me have a look at it! It's true that the BTreeMap representation has the advantage of being deterministic so great for testing, let's see what we can do there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is moving in the right direction :)
The main suggested changes are about
- Accepting all unreduced forms (even those where some variables have a canonical coefficient of 0) to remove linear complexity in addition and subtraction
- Move canonicalisation to the edges: Display and communication with the back end (I'll have a think about the latter, maybe it's not even necessary)
@eupn I had a quick go here https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=07c263548f34a377c3497a8b94d62a32 |
Got it, thanks a lot! |
Hey @eupn I realise we don't tell contributors to do it but we use https://github.com/rust-lang/rustfmt. |
Hey @eupn I think we're getting there! just a bit of context in the last change: the breaking test on 01c4d18 was due to the fact that that equality on programs (which is being tested) is derived automatically, so two lincomb that are "equivalent" (ie have the same canonical representation) are not "equal" (as they can have different representation in a vector of pairs). Therefore instead of making the optimizer canonical, I changed the definition of equality between lincomb to canonical equivalence. Let me know if you have any thoughts or I missed something! |
@eupn and on this last change, I made sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Resolves #312.
Replaced
BTreeMap
withVec
of pairs(var, value)
. Unit tests passed, although the order of sum items that was dependent on the order of keys inBTreeMap
is changed.?r @Schaeff