-
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?
IVS-137 ALB012 Vertical Segment Radius of Curvature #300
Conversation
Scenarios for validating the horizontal continuity were added. Those scenarios should be implemented in another rule, but were maintained in the commit to be evaluated, and should be altered after
…erkin-rules into IVS-137-ALB011-IfcAlignmentVerticalSegment.RadiusOfCurvature
It was created a function of comparison that considers the instance context precision tolerance. This function was added in the 'then' step that compares an attributes to an expression.
ALB010 is already in use. Renaming this feature will allow for a new rule ALB011 to check RadiusOfCurvature for the horizontal layout. (IVS-137)
…gment.RadiusOfCurvature
…ature' of https://github.com/fg55b/ifc-gherkin-rules into IVS-137-ALB012-IfcAlignmentVerticalSegment.RadiusOfCurvature
(IVS-143)
ALB012 focuses solely on 'RadiusOfCurvature' design parameter and includes a scenario that fixes #241. (IVS-137)
features/steps/thens/attributes.py
Outdated
'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, |
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 guess with str.split
these operator keys can never be matched. Not a problem for now, but maybe remove them in order not to give false hope.
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.
equal to
is being matched for the second scenario so should be OK to leave as-is
features/ALB012_Alignment-vertical-segment-radius-of-curvature.feature
Outdated
Show resolved
Hide resolved
features/steps/thens/attributes.py
Outdated
try: | ||
expression_value = eval(expression) | ||
except Exception as e: | ||
raise ValueError(f"Error evaluating expression: {e}") |
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've asked this before but what's currently the behaviour of raising exceptions?
What happens for example with a ZeroDivisionError
.
Regarding that specifically should there be some rules to prevent zeros in the denominator in this equation?
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.
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.
It would be nice to have something in the decorator or validation_handling that could return an ERROR outcome whenever an exception is raised. I started to replace all of the raise
statements with yield ValidationOutcome(... ValidationSeverity.ERROR)
but that felt like a non-pythonic brute force approach.
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 happens for example with a ZeroDivisionError.
It is not caught in CI/CD. There is nothing in place to handle the exception and therefore the process fails before it can yield a ValidationOutcome with severity ERROR. Current pytest logic is still 0 errors = pass
, not all results are ValidationOutcome(ValidationSeverity.PASSED)
.
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.
resolved in 6a3c82f
|
||
Scenario: Validating the absence of curvature radius for constant gradient vertical segment | ||
Given PredefinedType != 'ARC' or 'PARABOLICARC' | ||
Then The value of attribute RadiusOfCurvature must be empty |
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
Scenario Outline: Agreement of the segments of alignment | |
Given an IfcAlignmentSegment | |
Given The element nests an <AlignmentComponentType> | |
Then The type of attribute DesignParameters must be <SegmentType> | |
Examples: | |
| AlignmentComponentType | SegmentType | | |
| IfcAlignmentHorizontal | IfcAlignmentHorizontalSegment | | |
| IfcAlignmentVertical | IfcAlignmentVerticalSegment | | |
| IfcAlignmentCant | IfcAlignmentCantSegment | |
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:
Are we sure that violations of ALB002 don't cause crashes in ALB012 causing the entire validation task to fail?
Yes, we are sure now because of the additional Given statement in the feature's Background.
features/steps/thens/attributes.py
Outdated
|
||
@gherkin_ifc.step('First instance {first_expression} value must be {comparison_op} the second instance {second_expression}') | ||
@gherkin_ifc.step('First instance {first_expression} value must be {comparison_op} the second instance {second_expression} at depth 1') | ||
def step_impl(context, inst, first_expression:str, comparison_op:str, second_expression:str): |
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 don't see a usage of this impl function
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.
Correct, and thanks for the catch. We originally had a number of additional scenarios and this one handles the check of StartDistAlong
that I mentioned in Slack. See also IVS-150 in JIRA. I would like to leave it in for now so that it's there fore future rules.
After another look I see that there is some duplicated code and there is likely benefit in using itertools.piecewise()
for consistency with ALS implementation. I've removed this function and stored in a gist for the time being at https://gist.github.com/civilx64/9d533dc44d6b2fa4a4ef21bd79963a3c.
features/steps/utils/geometry.py
Outdated
'less than or equal to'. | ||
''' | ||
if comparison_operator == 'equal to': | ||
return math.isclose(value_1, value_2, abs_tol=precision) |
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.
rel_tol
has a default value. Should we set it to 0
? https://docs.python.org/3/library/math.html#math.isclose
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.
From the docs:
rel_tol must be greater than zero.
The implementation seems to be correct as-is.
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 code disagrees. I really prefer to be explicit about this. If we want a relative tolerance in addition to an absolute tolerance I prefer that we write it down in the code as an act of explicitly documenting our behaviour. I think only an absolute tolerance better matches the intent of the IFC spec (even if that's very limited).
>>> import math
>>> math.isclose(1.0, 1.0, rel_tol=0.)
True
>>> math.isclose(1.0, 1.0, rel_tol=-1.0)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: tolerances must be non-negative
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 see. Implemented in 515cc49.
Co-authored-by: Thomas Krijnen <[email protected]>
A few things to discuss in order to wrap up the PR:
|
- adds a Given statement for proper entity type of DesignParameters - adds `na` unit test file to confirm this new Given statement - adds `na` unit test file to confirm rule is not activated for alignment with horizontal layout only (IVS-137)
) | ||
@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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
“The value of attribute must be equal to the expression: 0”
While maybe not ideal, this pattern would be fine IMHO.
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 first try to fit it into
Could be as simple as:
>>> import ast
>>> required = '1.0'
>>> try: required = ast.literal_eval(required)
... except: pass
...
>>> type(required)
<class 'float'>
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 comment
The reason will be displayed to describe this comment to others. Learn more.
“The value of attribute must be equal to the expression: 0”
While maybe not ideal, this pattern would be fine IMHO.
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.
I would first try to fit it into
Could be as simple as:
>>> import ast >>> required = '1.0' >>> try: required = ast.literal_eval(required) ... except: pass ... >>> type(required) <class 'float'>
But maybe also take into account tolerances when you detect you're comparing floats
The comparison using tolerances, after evaluating the string, can be done using
def compare_with_precision(value_1: float, value_2: float, precision: float, comparison_operator: str) -> bool: |
However, the expression as is would only compare using the "equal to" operator, which is not applicable to this case.
features/steps/utils/geometry.py
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of the following?
operators = {
'equal to': lambda: math.isclose(value_1, value_2, abs_tol=precision),
'not equal to': lambda: not math.isclose(value_1, value_2, abs_tol=precision),
'greater than': lambda: value_1 > value_2 and not math.isclose(value_1, value_2, abs_tol=precision),
'less than': lambda: value_1 < value_2 and not math.isclose(value_1, value_2, abs_tol=precision),
'greater than or equal to': lambda: value_1 > value_2 or math.isclose(value_1, value_2, abs_tol=precision),
'less than or equal to': lambda: value_1 < value_2 or math.isclose(value_1, value_2, abs_tol=precision)
}
if comparison_operator not in operators:
raise ValueError(f"Invalid comparison operator: {comparison_operator}")
return operators[comparison_operator]()
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Actually worse IMO. Replaced with pattern matching in f6fc490.
'less than or equal to' : operator.le, | ||
} | ||
|
||
if expression is not None: |
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.
Can probably be simply
if expression is not None: | |
if expression: |
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.
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, | ||
} |
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.
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.
Maybe not in the scope of this PR, but what about a ticket with these kind of cleanup/maintanance tasks?
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.
Ticket is good. This PR is turning into a marathon and I'd like to wrap up.
…m/buildingSMART/ifc-gherkin-rules into IVS-137-ALB012-IfcAlignmentVerticalSegment.RadiusOfCurvature
The test file that raises an exception should start with 'fail'. (IVS-137)
…gment.RadiusOfCurvature
Exception handling added in 6a3c82f. Thanks @Ghesselink! |
Fixes #241
Also adds an additional check per Note 3 on the
RadiusOfCurvature
attribute in theIFC 4.3.2.0 schema documentation.