Fixes for problems that occured during my master thesis #8
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)
…ption that constraint is always >= does not hold for random uvl expressions)
…hes for expressions as direct constraints but not as expressions that are part of a normal constraint
…e (old one was not wrong)
…r (e.g. for - in name)
…r (e.g. for - in name)
…placement, like described in UVL BA)
…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
SundermannC
left a comment
SundermannC
left a comment
There was a problem hiding this comment.
Looks good. Mostly some questions and still need to do perform some tests when running the updated version.
| <groupId>org.antlr</groupId> | ||
| <artifactId>antlr4-runtime</artifactId> | ||
| <version>4.13.1</version> | ||
| </dependency> |
There was a problem hiding this comment.
As discussed internally, we should revert the changes to the dependencies
There was a problem hiding this comment.
I agree. But keep in mind, that there currently is a bug in the uvl-parser version in the maven repo.
| LiteralConstraint subTreeRootConstraint = new LiteralConstraint(newChild); | ||
| newConstraint = new ImplicationConstraint(subTreeRootConstraint, new ParenthesisConstraint(newLiteral));; | ||
| } | ||
| }else{ |
There was a problem hiding this comment.
Unusual formatting of the else. Why no spaces?
| } | ||
| } | ||
| /* | ||
| additional constraint with all cloned versions ored |
There was a problem hiding this comment.
Do we need this commented-out fragmnet?
There was a problem hiding this comment.
This code was used to create an additional constraint, where are cloned features are or-ed. For a constraint that contains a cloned feature, it can then be replaced by this expression. We discussed this kind of constraint while talking about the semantic of constraints regarding feature cardinality. However, I did not include this kind of constraint and therefore commented it out again. I did not remove it, in case we want to use it later on.
| } | ||
| constraint.replaceConstraintSubPart(subPart, new ParenthesisConstraint(newOr)); | ||
| } | ||
| }else { |
| if (subTreeFeatures.contains(feature)) { | ||
| return true; | ||
| } | ||
| }else if (constraint instanceof ExpressionConstraint) { |
| if (constraint instanceof ExpressionConstraint) { | ||
| adaptExpression(((ExpressionConstraint) constraint).getLeft(), featureReplacementMap); | ||
| adaptExpression(((ExpressionConstraint) constraint).getRight(), featureReplacementMap); | ||
| }else{ |
| for (Constraint constraint : constraints) { | ||
| orConstraint.add_sub_part(constraint); | ||
| } | ||
| /* |
There was a problem hiding this comment.
No, I guess not. Thats the original code before introducing MultiOrs. If we are fine with keeping the MultiOr, I can remove it.
| BOOLEAN_LEVEL(1, Constants.BOOLEAN_LEVEL), | ||
| ARITHMETIC_LEVEL(3, Constants.ARITHMETIC_LEVEL), | ||
| TYPE_LEVEL(5, Constants.TYPE_LEVEL), | ||
| TYPE_LEVEL(7, Constants.TYPE_LEVEL), |
There was a problem hiding this comment.
Can you explain the idea behind this change?
There was a problem hiding this comment.
Before the change, all minor language levels had the same priority. Meaning it did not matter in which order they were converted. However, for feature cardinality and aggregate function it does make a difference in which order they are converted. In the implementation major levels have an odd number and minor levels an even number. If Arithmetic level is 3and typelevel is 5, there is only one even number in between. To give aggregate function and type level different prioritys (give them different even numbers), I increased the value of the type level. If there are more typelevels added to UVL this rather simple approach of even and odd numbers should maybe be refactored.
| @@ -0,0 +1,97 @@ | |||
| package de.vill.model.constraint; | |||
There was a problem hiding this comment.
We should probably remove Regular OrConstraint altogether (not yet necessarily). Or does keeping it have any benefit?
There was a problem hiding this comment.
I agree, that removing it complete might be a good idea. However, I did not do it yet, because I was unsure if there are others depending on it.
SundermannC
merged commit 6cd2983
into
Universal-Variability-Language:refactoring_metamodel
2 participants
No description provided.