Refactor constraints, conversion strategies, and print #18
Conversation
…o-boolean constraints in .opb format
…ssible group cardinality conversions
…nto refactoring_metamodel
… during conversion strategy
…ontain doubles by encoding them in integer values
…ndled as literal times literal)
… other mathematical operators)
…ure cardinality subtree roots, expression contains feature from feature cardinality, unsatisfiable expression should return false and not throw error, several issues with opb division encoding, removed unnecessary overwrites of left and right in expression constraints, force order of featurecardinality and aggregate function conversion, several to string methods for better debugging, issues with whole number encoding because numbers get larger than long value, uniform substitution, simpler substitution code,
…vs tseitin style cross-tree constraint encoding)
…ctoring_metamodel
Fixes for problems that occured during my master thesis
SundermannC
changed the title
|
This PR comes with several changes to simplify the usage of constraints, conversion strategies, and printing. Constraints
Conversion strategies:
Printing:
|
coemgen1992
left a comment
coemgen1992
left a comment
There was a problem hiding this comment.
I only have some comments regarding comments and guarantees, which should be resolved. Besides that, the code looks good.
| } | ||
| } | ||
| /* | ||
| additional constraint with all cloned versions ored |
There was a problem hiding this comment.
Is there a particular reason to leave the comments in?
Please remove these comments, otherwise.
| Expression left = ((ExpressionConstraint) constraint).getLeft(); | ||
| Expression right = ((ExpressionConstraint) constraint).getRight(); | ||
| return expressionContains(left, subTreeFeatures) || expressionContains(right, subTreeFeatures); | ||
| } |
There was a problem hiding this comment.
This if-else statement is very specific.
Can we guarantee that there only will be these cases? If not, we may want to add some sort of log, error statement in an else branch of the cascade.
There was a problem hiding this comment.
I think this makes sense. It should only be those three cases in this abstraction level:
- Found the atom to collect it
- Constraint is an expression, so we switch to "expression mode" for the traversal
- Traverse all subparts of the constraint
| orConstraint.add_sub_part(constraint); | ||
| } | ||
| /* | ||
| Constraint orConstraint; |
There was a problem hiding this comment.
Same comment as before. Please remove code in comments.
| orConstraint.add_sub_part(constraint); | ||
| } | ||
| /* | ||
| Constraint orConstraint; |
| static { | ||
| constraintprecedenceLookup = new HashMap<>(); | ||
|
|
||
| // n-ary |
There was a problem hiding this comment.
I am always a bit nervous on magic numbers in code.
Consider making the ordering clear with constants?
| StringBuilder result = new StringBuilder(); | ||
| for (Constraint part : sub_parts) { | ||
| result.append(AutomaticBrackets.enforceConstraintBracketsIfNecessary(this, part, withSubmodels, currentAlias)); | ||
| result.append(" | "); |
There was a problem hiding this comment.
Do we want to introduce a Symbol lookup at some point?
|
All comments should be addressed now. Pipeline sadly still fails due to the publishing issues. Locally all tests ran through. |
|
@SundermannC I will try to run it by myself next week. |
|
LGTM, nothing broke and code looks clean. |
No description provided.