Skip to content
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-80/GRD000 - Grid Information #257

Draft
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

Ghesselink
Copy link
Contributor

This one is a bit more elaboration and includes changes in the python implementation

General/improvements not limited to the current rule:

  • Added a return to entity statement used to navigate within the concept templates in gherkin by returning to a previous layer in the context stack. This statement was previously used for an unmerged pull request, and updated to work with the decorator. One caveats is that I'm only confident in using this statement for activation purposes, not for normative rules due to the risk validating instances that are filtered out by previous given statement (as we go back to the point in the context stack where they were not filtered out yet). It would also be nice to simply go back X amount of statements, or repeat previous steps.
  • The activation instances issue of GEM051 is now handled generically. We used activation_instances[top_level_index] twice which was redundant. I've tested that GEM051 has the same activation instances as before.
  • Some cleanup was done to keep the logic of including/excluding subtypes in our statements in one location (we're re-using it for GRD000).

GRD000 specific:

  • Again, ensured that non-activated files have no instances when the "then" step is reached, and vice versa; activated files has context.instances populated at the same point..
  • Added a file that tests the return statement by being correct except for the absence of IfcGrid (which is in the middle of the sequence of statements)
  • Three files created with IfcOpenShell, one from the IFC4.3x sample files repo.

@Ghesselink Ghesselink added the rule tracker for development of a new rule/warning label Aug 7, 2024
@Ghesselink Ghesselink marked this pull request as draft August 8, 2024 15:44
@version1
@E00020

Feature: GRD000 - Grid Information
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Feature: GRD000 - Grid Information
Feature: GRD000 - Grid Placement

@evandroAlfieri Why did we pick "Information" instead of "Placement"? Grid Information to me hints at information on grids in the model, not other elements placed according to grid.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I thought moving the pr back to draft was enough to save you from reviewing it. It's about this rule that we (Geert and I) wanted to talk with you on Monday

Comment on lines +15 to +16
Given its attribute PlacesObject
Given its entity type is 'IfcProduct' including subtypes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is governed by the schema.

PlacesObject: SET [0:?] OF IfcProduct FOR ObjectPlacement

But what is not governed is that there is at least one Product being placed by the Placement, which I think is a good requirement to add here as an activation criterion.

Comment on lines +18 to +20
Given its attribute PlacementRelTo
Given its attribute PlacesObject
Given its entity type is 'IfcGrid'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three lines should actually not be a given statement, but a then statement.

Given an IfcGridPlacement Then PlacementRelTo/PlacesObject **must be** IfcGrid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it a normative rule, and not a rule to check for activation.
Which is part of the reason why this PR is de-activated. It would probably be better to create two new normative rules and then skip the 000-rule. Based on this issue

Given its attribute PlacesObject
Given its entity type is 'IfcProduct' including subtypes
Given return to IfcGridPlacement
Given its attribute PlacementRelTo
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PlacementRelTo does not exist on schemas pre IFC4.3. I think this must be reflected in the scenario.

Comment on lines +22 to +23
Given its attribute PlacementLocation
Given IntersectingAxes = not empty
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is given by the schema:

IntersectingAxes: LIST [2:2] OF UNIQUE IfcGridAxis

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I was wondering how close we should stay to the relating concept template.

Given its attribute PlacementLocation
Given IntersectingAxes = not empty

Then The IFC model contains information on quantities of elements
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy pasta

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of which, maybe this is too error prone and we can rather easily replace this with:

Suggested change
Then The IFC model contains information on quantities of elements
Then The IFC model contains information on the selected functional part

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undercooked pasta, in this case ..

It's indeed very easy to miss, and maybe not such an important part. We have the description in the feature file to provide context after all.

Given IntersectingAxes = not empty

Then The IFC model contains information on quantities of elements

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So all in all, you could consider rewriting this scenario as:

Given an IfcProduct
And its attribute ObjectPlacement
And its entity type is IfcGridPlacement

Then The IFC model contains information on ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But would this be close enough to the relating concept template?
Would it be nice if we could state that some software supports a functional part based on these templates?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it goes back to the age old question that we have to second guess the intention behind the template, and this case I'd say it's twofold:

  • [GRD000] Describe grid placement functionality: IfcProduct ----ObjectPlacement---> GridPlacement
  • [GRD001-a] Constrain placement of grid "a grid placement should be defined relatively to a grid": GridPlacement ----PlacementRelTo----> IfcObjectPlacement ---PlacesObject---> IfcGrid

And then a third component that is important, but is not in the template:

  • [GRD001-b] "The grid to which the placement is relative should contain the axes that are used for the intersection"

afbeelding

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's phrase it differently, as soon as the model contains:

IfcProduct ----ObjectPlacement---> GridPlacement

The file contains a "grid placement" according to the functional part.

But if it's not according to the 2 further specifications I just highlighted, it's not that the model doesn't contain it, it's that the usage is invalid. So yes, these are additional normative parts and we need to create this separation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evandroAlfieri @Ghesselink Annother rule for the backlog is this informal proposition from the docs:

IntersectingAxes[1] and IntersectingAxes[2] shall not be part of the same row of grid axes, i.e. both shall not be within the same set of IfcGrid.UAxes or IfcGrid.VAxes of the corresponding IfcGrid.

https://ifc43-docs.standards.buildingsmart.org/IFC/RELEASE/IFC4x3/HTML/lexical/IfcVirtualGridIntersection.htm

return layer if context.include_layer else None

#ensure the stack does not get pupulated when nothing was yielded in the last step
if (lambda f: f(f))(lambda f: lambda data: bool(data) and (not isinstance(data, (list, tuple)) or any(f(f)(item) for item in data)))(context.instances):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again this is rocket science to me. Can we clear this up first in the other PR

Copy link
Contributor Author

@Ghesselink Ghesselink Aug 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's bring the rocket science back to building information science then 🚀
See #258 (comment)
In short, it ensures that context.instances is meaningful. In this case, when the result of the last Given statement is an empty set of instances, we don't want to go back. For instance;

| Step  ->  context.instances after step |
Given An IfcEntity -> [Entity, Entity, Entity]
Given its attribute A -> [SomeAttr, SomeAttr, SomeOtherAttr]
Given its attribute is SomeAttr ->  [SomeAttr, SomeAttr, ()]
Given its attribute B ->  [(), (), False]
Given its attribute IfcEntity -> What do we return here?

In case we don't check whether context.instances is meaningful, we'd return the context.instances as was the case after the initial Given step. This is not the preferred behavior, since the preoccuring Given statements determined that the rule would not be applicable.

The handle_given section in the decorator would probably be a better location for this logic.

@@ -203,6 +200,7 @@ def should_apply(items, depth):

# evokes behave error
generate_error_message(context, [gherkin_outcome for gherkin_outcome in context.gherkin_outcomes if gherkin_outcome.severity in [OutcomeSeverity.WARNING, OutcomeSeverity.ERROR]])
pass
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A debug statement I forgot to remove, sorry
4e3ce44

Comment on lines +63 to +66
grid_placement.PlacementLocation = f.createIfcVirtualGridIntersection(
[grid_axis_1, grid_axis_2],
(0.0, 0.0, 0.0)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates the informal propositions here: https://ifc43-docs.standards.buildingsmart.org/IFC/RELEASE/IFC4x3/HTML/lexical/IfcVirtualGridIntersection.htm

It's also a really creative grid.

I would define:

  U axes:
    (0, 0) -> (0, 2)
    (1, 0) -> (1, 2)
    (2, 0) -> (2, 2)
  V axes:
    (0, 0) -> (2, 0)
    (0, 1) -> (2, 1)
    (0, 2) -> (2, 2)
  W:
    <empty>

And then you intersect e.g UAxes[1] with VAxes[1], but they need to be instances from the grid itself.

Also you don't actually reference this variable, but redefined sth on L 777

Also the polyline grid axes are best 2d instead of 3d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule tracker for development of a new rule/warning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants