-
Notifications
You must be signed in to change notification settings - Fork 18
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
IVS-137 ALB012 Vertical Segment Radius of Curvature #300
base: development
Are you sure you want to change the base?
Changes from 15 commits
1d2c578
9bd31c8
bc9c3e8
2e49010
fc93553
b89ae45
9969602
7670846
50ace43
7f21949
29922bd
f5f6237
3b6122a
515cc49
42959f2
d477990
21a5c6f
6a3c82f
3de923b
d4190bc
c382fe7
f6fc490
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
@implementer-agreement | ||
@ALB | ||
@version1 | ||
@E00020 | ||
Feature: ALB012 - Alignment vertical segment radius of curvature | ||
The rule verifies the 'RadiusOfCurvature' design parameter for vertical alignment segments. | ||
|
||
Background: | ||
Given A model with Schema "IFC4.3" | ||
Given An IfcAlignmentVertical | ||
Given A relationship IfcRelNests from IfcAlignmentVertical to IfcAlignmentSegment and following that | ||
Given Its attribute DesignParameters | ||
Given Its entity type is 'IfcAlignmentVerticalSegment' | ||
|
||
Scenario: Validating the absence of curvature radius for specific predefined types of vertical segment | ||
Given PredefinedType != 'ARC' or 'PARABOLICARC' | ||
Then The value of attribute RadiusOfCurvature must be empty | ||
|
||
Scenario: Validating the radius of curvature for parabolic segments | ||
Given PredefinedType = 'PARABOLICARC' | ||
Then The value of attribute RadiusOfCurvature must be equal to the expression: HorizontalLength / ( EndGradient - StartGradient ) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,5 +1,5 @@ | ||||||
import operator | ||||||
|
||||||
import ifcopenshell | ||||||
|
||||||
from utils import misc, system, geometry | ||||||
from validation_handling import gherkin_ifc | ||||||
|
@@ -60,37 +60,124 @@ def accumulate_errors(i): | |||||
yield from errors | ||||||
|
||||||
|
||||||
@gherkin_ifc.step('The value of attribute {attribute} must be {value}') | ||||||
@gherkin_ifc.step('The value of attribute {attribute} must be {value} {display_entity:display_entity}') | ||||||
def step_impl(context, inst, attribute, value, display_entity=0): | ||||||
# @todo the horror and inconsistency.. should we use | ||||||
# ast here as well to differentiate between types? | ||||||
pred = operator.eq | ||||||
if value == 'empty': | ||||||
value = () | ||||||
elif value == 'not empty': | ||||||
value = () | ||||||
pred = operator.ne | ||||||
elif 'or' in value: | ||||||
opts = value.split(' or ') | ||||||
value = tuple(opts) | ||||||
pred = misc.reverse_operands(operator.contains) | ||||||
|
||||||
if isinstance(inst, (tuple, list)): | ||||||
inst = inst[0] | ||||||
attribute_value = getattr(inst, attribute, 'Attribute not found') | ||||||
if attribute_value is None: | ||||||
attribute_value = () | ||||||
if inst is None: | ||||||
# nothing was activated by the Given criteria | ||||||
yield ValidationOutcome(inst=inst, severity=OutcomeSeverity.EXECUTED) | ||||||
elif not pred(attribute_value, value): | ||||||
yield ValidationOutcome( | ||||||
inst=inst, | ||||||
expected=None if not value else value, | ||||||
observed=misc.recursive_unpack_value(attribute_value), | ||||||
severity=OutcomeSeverity.ERROR | ||||||
) | ||||||
@gherkin_ifc.step('The value of attribute {attribute} must be {value_or_comparison_op}') | ||||||
@gherkin_ifc.step('The value of attribute {attribute} must be {value_or_comparison_op} {display_entity:display_entity}') | ||||||
@gherkin_ifc.step('The value of attribute {attribute} must be {value_or_comparison_op} the expression: {expression}') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think that using this with constant values directly in place of the expression would be confusing? For example, “The value of attribute must be equal to the expression: 0”? I ask because, in ALB011, the StartRadiusOfCurvature and EndRadiusOfCurvature of IfcAlignmentHorizontalSegment need to be zero or non-zero according to its PredefinedType. This cannot be verified with the first expression because "The value of attribute must be 0.0" would compare the 0.0 float value of the IFC file with the 0.0 string value declared on the rule. It also does not consider the precision. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
While maybe not ideal, this pattern would be fine IMHO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would first try to fit it into Could be as simple as:
But maybe also take into account tolerances when you detect you're comparing floats There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This would be particularly useful to evaluate if StartRadiusOfCurvature and EndRadiusOfCurvature of IfcAlignmentHorizontalSegment is not zero for nonlinear PredefinedTypes, as the "not equal to" operator can be used.
The comparison using tolerances, after evaluating the string, can be done using
However, the expression as is would only compare using the "equal to" operator, which is not applicable to this case. |
||||||
def step_impl(context, inst, attribute:str, value_or_comparison_op:str, expression:str=None, display_entity=0): | ||||||
""" | ||||||
Compare an attribute to an expression based on attributes. | ||||||
|
||||||
The {comparison_op} operator can be 'equal to', 'not equal to', 'greater than', 'less than', 'greater than or equal to', and 'less than or equal to'. | ||||||
|
||||||
The {expression} should be composed by attribute values, and use the following operators: | ||||||
+ : addition; | ||||||
- : subtraction; | ||||||
* : multiplication; | ||||||
/ : division; | ||||||
% : modulus; | ||||||
** : exponentiation. | ||||||
""" | ||||||
|
||||||
operators = { | ||||||
'+' : operator.add, | ||||||
'-' : operator.sub, | ||||||
'*' : operator.mul, | ||||||
'/' : operator.truediv, | ||||||
'%' : operator.mod, | ||||||
'**' : operator.pow, | ||||||
'equal to' : operator.eq, | ||||||
'not equal to' : operator.ne, | ||||||
'greater than' : operator.gt, | ||||||
'less than' : operator.gt, | ||||||
'greater than or equal to' : operator.ge, | ||||||
'less than or equal to' : operator.le, | ||||||
} | ||||||
Comment on lines
+81
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're using operators at quite some points in the code now. What do you think of moving them to a dataclass and seperating them into arithmetic operators, comparison operators and the various interpretations of operators. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ticket is good. This PR is turning into a marathon and I'd like to wrap up. |
||||||
|
||||||
if expression is not None: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can probably be simply
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
# Get compared attribute value | ||||||
|
||||||
attr_compared_value = getattr(inst, attribute, 'Compared attribute not found') | ||||||
if isinstance(attr_compared_value, ifcopenshell.entity_instance): | ||||||
raise Exception('Compared attribute value is an IFC entity') | ||||||
|
||||||
# Replace attribute names with attribute values in the expression | ||||||
for string_content in expression.split(): | ||||||
# Checks if the string is not a operator neither parenthesis | ||||||
if string_content not in [*operators, '(', ')']: | ||||||
if hasattr(inst, string_content): | ||||||
if not isinstance(getattr(inst, string_content), ifcopenshell.entity_instance): | ||||||
expression = expression.replace(string_content, str(getattr(inst, string_content))) | ||||||
else: | ||||||
raise Exception('Expression attribute value is an IFC entity') | ||||||
else: | ||||||
raise Exception('Expression attribute not found') | ||||||
|
||||||
# Evaluate the string expression using eval | ||||||
try: | ||||||
expression_value = eval(expression) | ||||||
except Exception as e: | ||||||
raise ValueError(f"Error evaluating expression: {e}") | ||||||
|
||||||
# Compare the attribute with the expression value, considering the precision | ||||||
entity_contexts = geometry.recurrently_get_entity_attr(context, inst, 'IfcRepresentation', 'ContextOfItems') | ||||||
precision = geometry.get_precision_from_contexts(entity_contexts) | ||||||
|
||||||
try: | ||||||
result = geometry.compare_with_precision( | ||||||
attr_compared_value, expression_value, precision, value_or_comparison_op | ||||||
) | ||||||
if result: | ||||||
yield ValidationOutcome( | ||||||
inst=inst, | ||||||
expected=f"A value {value_or_comparison_op} {expression_value} with precision {precision}", | ||||||
observed={attr_compared_value}, | ||||||
severity=OutcomeSeverity.PASSED, | ||||||
) | ||||||
else: | ||||||
yield ValidationOutcome( | ||||||
inst=inst, | ||||||
expected=f"A value {value_or_comparison_op} {expression_value}", | ||||||
observed={attr_compared_value}, | ||||||
severity=OutcomeSeverity.ERROR, | ||||||
) | ||||||
except ValueError as e: | ||||||
yield ValidationOutcome( | ||||||
inst=inst, | ||||||
expected=f"A value {value_or_comparison_op} {expression_value}", | ||||||
observed=f"Error during comparison: {e}", | ||||||
severity=OutcomeSeverity.ERROR, | ||||||
) | ||||||
|
||||||
|
||||||
else: | ||||||
# @todo the horror and inconsistency.. should we use | ||||||
# ast here as well to differentiate between types? | ||||||
pred = operator.eq | ||||||
if value_or_comparison_op == 'empty': | ||||||
value_or_comparison_op = () | ||||||
elif value_or_comparison_op == 'not empty': | ||||||
value_or_comparison_op = () | ||||||
pred = operator.ne | ||||||
elif 'or' in value_or_comparison_op: | ||||||
opts = value_or_comparison_op.split(' or ') | ||||||
value_or_comparison_op = tuple(opts) | ||||||
pred = misc.reverse_operands(operator.contains) | ||||||
|
||||||
if isinstance(inst, (tuple, list)): | ||||||
inst = inst[0] | ||||||
attribute_value = getattr(inst, attribute, 'Attribute not found') | ||||||
if attribute_value is None: | ||||||
attribute_value = () | ||||||
if inst is None: | ||||||
# nothing was activated by the Given criteria | ||||||
yield ValidationOutcome(inst=inst, severity=OutcomeSeverity.EXECUTED) | ||||||
elif not pred(attribute_value, value_or_comparison_op): | ||||||
yield ValidationOutcome( | ||||||
inst=inst, | ||||||
expected=None if not value_or_comparison_op else value_or_comparison_op, | ||||||
observed=misc.recursive_unpack_value(attribute_value), | ||||||
severity=OutcomeSeverity.ERROR | ||||||
) | ||||||
|
||||||
@gherkin_ifc.step('The {field} of the {file_or_model} must be "{values}"') | ||||||
def step_impl(context, inst, field, file_or_model, values): | ||||||
|
@@ -134,7 +221,7 @@ def step_impl(context, inst, constraint, num): | |||||
inst = str(inst) | ||||||
op = misc.stmt_to_op(constraint) | ||||||
if not op(len(inst), num): | ||||||
yield ValidationOutcome(inst=inst, expected=num, observed=len(inst), severity=OutcomeSeverity.ERROR) | ||||||
yield ValidationOutcome(inst=inst, expected=num, observed=len(inst), severity=OutcomeSeverity.ERROR) | ||||||
|
||||||
|
||||||
@gherkin_ifc.step('The characters must be within the official encoding character set') | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -222,3 +222,31 @@ def alignment_segment_angular_difference( | |
delta = abs(current_start_direction - preceding_end_direction) | ||
|
||
return delta | ||
|
||
|
||
def compare_with_precision(value_1: float, value_2: float, precision: float, comparison_operator: str) -> bool: | ||
""" | ||
Compare the value_1 with value_2 according to a comparison operator, considering a precision tolerance. | ||
|
||
The valid comparison operators are: | ||
'equal to'; | ||
'not equal to'; | ||
'greater than'; | ||
'less than'; | ||
'greater than or equal to'; | ||
'less than or equal to'. | ||
""" | ||
if comparison_operator == 'equal to': | ||
return math.isclose(value_1, value_2, rel_tol=0., abs_tol=precision) | ||
elif comparison_operator == 'not equal to': | ||
return not math.isclose(value_1, value_2, rel_tol=0., abs_tol=precision) | ||
elif comparison_operator == 'greater than': | ||
return value_1 > value_2 and not math.isclose(value_1, value_2, rel_tol=0., abs_tol=precision) | ||
elif comparison_operator == 'less than': | ||
return value_1 < value_2 and not math.isclose(value_1, value_2, rel_tol=0., abs_tol=precision) | ||
elif comparison_operator == 'greater than or equal to': | ||
return value_1 > value_2 or math.isclose(value_1, value_2, rel_tol=0., abs_tol=precision) | ||
elif comparison_operator == 'less than or equal to': | ||
return value_1 < value_2 or math.isclose(value_1, value_2, rel_tol=0., abs_tol=precision) | ||
else: | ||
raise ValueError(f"Invalid comparison operator: {comparison_operator}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of the following?
Perhaps it's a bit easier to read than if/else statements There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually worse IMO. Replaced with pattern matching in f6fc490. |
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.
The correspondence of
IfcAlignmentVertical <- nests -> Segment -> IfcAlignmentVerticalSegment
is assessed in ALB002. But we don't have rule dependencies. Are we sure that violations of ALB002 don't cause crashes in ALB012 causing the entire validation task to fail?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.
ALB002 is currently disabled with an item in backlog to be re-activated. I added a link to this comment to the ALB002 reactivation ticket IVS-75 so that I remember to verify independence of ALB002 and ALB012 in UAT.
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.
ALB002 is currently deactivated.
We indeed don't have rule dependencies (should we?). I did some experiments, activated the rule again and ensured that ALB002 always fails when the step you mentioned is reached. The result is that both rules (ALB012 and ALB002) fail
I reviewed the test files for ALB002 and ran them on a deployment of the gherkin repository, which included ALB012. It seems that ALB012 runs even when the correspondence fails.
IIRC, in case there is an error unrelated to the rule (e.g. a
ZeroDivisionError
as mentioned earlier), it's a bit annoying for the gherkin CI/CD but without consequences for subsequent rules.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.
That appears even worse. Is this logic never checked?
ifc-gherkin-rules/features/ALB002_Alignment-layout.feature
Lines 53 to 64 in 31df653
I'm just worried about the AttributeErrors when DesignParameters is not of type IfcAlignmentVerticalSegment
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 agree, for some reason I thought it was IfcAlignmentVerticalSegment that nests IfcAlignmentVertical and that IfcAlignmentSegment was a supertype.
Do we have an implementation "Given attribute is of type {entity_type}? I looked briefly earlier and didn't find anything.Edit: I found the Given and added it to the background.
Regardless of where this lands I will add additional fail files so that we are certain that the type check is handled correctly for both ALB002 and ALB012.I added a test file
na-alb012-incorrect_type_for_DesignParameters_attirbute.ifc
. Because of the new Given in ALB012, the incorrect type will mean that ALB012 is not activated. However ALB002 will still capture this as an error. Or at least that is the design intent.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.
Coming back to the original question:
Yes, we are sure now because of the additional Given statement in the feature's Background.