-
Notifications
You must be signed in to change notification settings - Fork 16
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
an initial go at representing a model #15
Conversation
src/main.rs
Outdated
Statement::Declaration(&b_decision_variable), | ||
Statement::Declaration(&c_decision_variable), | ||
Statement::Constraint(Expression::Eq( | ||
Box::from(Expression::Sum(vec![a_reference, b_reference, c_reference])), |
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.
probably needs to be (rust) reference or else a/b/c cannot appear in multiple constraints...
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.
What about?
let x = DecisionVariable(x,IntDomain(1,10))
builder.addVariable(x.clone())
builder.addVariable(x)
I presume you can just check equality between different instances of an identical variable by comparing their member values. (I think you can just derive the Eq trait)
@lixitrixi and I are discussing lifetime and borrow issues relating to his builder code and it seems like this AST needs to be fully without references (moved not borrowed) otherwise issues occur. In particular with dangling pointers - things being made in a function, being added to a Vec<&foo>, then being deallocated when the function exits.
e.g.
fn find(mut self, name: String, domain: Domain) -> Self {
let decision_variable = DecisionVariable {
name: Name::UserName(name),
domain,
};
let statement: Statement = Statement::Declaration(decision_variable);
self.add_statement(statement)
}
but other examples exist too.
The alternative is put everything on the heap (Box
and so on).
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.
(Of course, nested expressions would still need to be in a Box
as they have indeterminate size)
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 am not sure. This is the kind of thing we have to try and see.
I think the builder is a red herring though, what matters is the data structure we end up with.
There are a bunch of operations we want to support efficiently and they will dictate what's the best design.
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 the builder is a red herring though, what matters is the data structure we end up with.
We are talking about the builder in particular as we are debugging it as we speak, but I think as the model object is long-lived it will lend itself to alot of dangling stack pointers.... (that is, unless we define everything thats inside the model at the top of main)
I see that having many many identical copies of DecisionVariable all around whenever you do a Constraint::Reference() (for example) is not ideal - maybe Rc
/ Box
are the way to go?
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 have been playing a bit more with this (in particular adding a >= b as a second constraint) and I have been leaning towards Rc for variable references (so Expression::Reference). Not saying this is the best thing to do yet!
The domain part definitely needs to be shared and any reasoninig done on this part should persist be visible to all copies.
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 would be tempted to put things inside an Rc (I'd put the Rc inside a class, so it's easy to change the insides later, all using an Rc really forces at this point is the need to 'clone').
Another that's worth thinking about early on I suggest, is if variables should have to be attached to a Model at the point they are created, or later on. Can variables have a life independent of a model? What if I put a variable into two models? That's fine if a variable is really just a placeholder (it is just treated as a label).
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.
Another reason to use an 'Rc', is I suggest if two variables with the same name and domain are created, they should be treated as two seperate variables, not treated as a single thing. In my experience, when building a big model it's easy to run out of names and start reusing, and people don't generally mean to be "clever". Also when making a big model, sometimes you make a variable that doesn't really even need a name (that's when you end up with things like 'aux297').
It would be fine, when you notice someone made two variables with the same name, to either warn them, or reject the model, but I wouldn't silently merge them.
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.
(one last post!)
I was curious how rustc did this so went to have a look :)
https://github.com/rust-lang/rust/blob/master/compiler/rustc_ast/src/ast.rs
Variables are just represented in the AST by a special 'symbol' type, after that's it's lots of enums, and Boxes (If you see 'P', that's a Box), for example down here we see the main 'ExprKind', which includes all the types of expressions: https://github.com/rust-lang/rust/blob/master/compiler/rustc_ast/src/ast.rs#L1399C16-L1399C16
I've copied this over to my fork! I will focus on parameterising (with a builder pattern, perhaps?) and refactoring over the next week. How does this sound? |
This is what it prints
|
Model { statements: [Declaration(RefCell { value: DecisionVariable { name: UserName("x"), domain: IntDomain([Bounded(1, 3)]) } }), Constraint(Eq(Sum([Reference(RefCell { value: DecisionVariable { name: UserName("x"), domain: IntDomain([Bounded(1, 3)]) } }), ConstantInt(2)]), ConstantInt(4)))] } |
I'll merge this so Felix can continue experimenting. Chris has some comments regarding RefCells, so we might revisit this. |
We are trying to represent:
Influenced by Conjure's internal representation, not sure about refs/boxes etc yet.
Creating PR to get feedback, will add simple testing before I am ready to merge.
Next steps:
conjure pretty --output-format=astjson
output