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

SPS002 - IfcFacility-part #65

Merged
merged 22 commits into from
Sep 21, 2023

Conversation

JTurbakiewicz
Copy link
Contributor

No description provided.

Copy link
Contributor

@pjanck pjanck left a comment

Choose a reason for hiding this comment

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

Initial comments.

test/files/sps2/fail-sps002.ifc Outdated Show resolved Hide resolved
test/files/sps2/fail-sps002.ifc Outdated Show resolved Hide resolved
test/files/sps2/pass-sps002-ifcfacility-aggregation.ifc Outdated Show resolved Hide resolved
features/SPS002_IfcFacility-aggregation.feature Outdated Show resolved Hide resolved
features/steps/thens/relations.py Outdated Show resolved Hide resolved
features/steps/thens/relations.py Outdated Show resolved Hide resolved
@Ghesselink
Copy link
Contributor

Looks good so far, not sure whether to look really in-depth since it is a draft PR (still WIP?).

I took the freedom to change the folder to 'sps002' and the filename for the fail file, since this caused the tests to fail. Now all the marks are green again and the autistic part of my brain can relax.

@JTurbakiewicz JTurbakiewicz changed the title [Draft] - SPS002 - IfcFacility-part SPS002 - IfcFacility-part May 14, 2023
@JTurbakiewicz JTurbakiewicz marked this pull request as ready for review May 14, 2023 21:51
features/SPS002_Correct_spatial_breakdown.feature Outdated Show resolved Hide resolved
features/SPS002_Correct_spatial_breakdown.feature Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this csv is incomplete. For example, I believe that IfcFacilityPart can be decomposed in IfcFacilityPart as well. Where does this csv originate from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@Ghesselink Ghesselink left a comment

Choose a reason for hiding this comment

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

I feel we need a re-organization regarding relationships. This PR, ALB004 and some other PRs are very similar in functionality which make it quite confusing. Right now we have :

  • nesting, aggegrating, containing, assigning relationships
  • An option to specify a relationship with a custom csv file
  • A couple of nuances (e.g., directly/indirectly, constraint on number of relating elements)
  • Slightly unstructured error classes

Maybe we could make a start with all punt the third point (the nuances) and place them into a single @then statement. Something like :

@then('Each {entity} must be {relationship} to {other_entity}
@then('Each {entity} must be {relationship}{if_asper} {condition}

The structure is then always:
Entity -> Relationship -> entity -> optional condition
And the more nuanced, 'advanced', statements are then :
Entity -> Relationship -> advanced requirement -> Entity
Some examples of optional condition :

  • 'As per {table}'
  • 'If {another_entity} is present'

And advanced requirements :
'at most 3 instance(s) of'
'only by the following entities:'
This condition we could parse as in #48

I feel this would result in clearer implementation and ease writing future rules and it's better to start with the cases that are the most similar (without the advanced statements). On the other hand, it will perhaps add more complication on github, as there are now many open PR and not a clear overview.
What do you think? @JTurbakiewicz

features/steps/thens/relations.py Outdated Show resolved Hide resolved
@JTurbakiewicz
Copy link
Contributor Author

JTurbakiewicz commented Jul 1, 2023

Added something along those lines as a test here: #67
What do you think? @Ghesselink

I'll be adding some more test cases there, so I'll most likely change some stuff still, but it's perhaps a reasonable start.

I feel we need a re-organization regarding relationships. This PR, ALB004 and some other PRs are very similar in functionality which make it quite confusing. Right now we have :

  • nesting, aggegrating, containing, assigning relationships
  • An option to specify a relationship with a custom csv file
  • A couple of nuances (e.g., directly/indirectly, constraint on number of relating elements)
  • Slightly unstructured error classes

Maybe we could make a start with all punt the third point (the nuances) and place them into a single @then statement. Something like :

@then('Each {entity} must be {relationship} to {other_entity}
@then('Each {entity} must be {relationship}{if_asper} {condition}

The structure is then always: Entity -> Relationship -> entity -> optional condition And the more nuanced, 'advanced', statements are then : Entity -> Relationship -> advanced requirement -> Entity Some examples of optional condition :

  • 'As per {table}'
  • 'If {another_entity} is present'

And advanced requirements : 'at most 3 instance(s) of' 'only by the following entities:' This condition we could parse as in #48

I feel this would result in clearer implementation and ease writing future rules and it's better to start with the cases that are the most similar (without the advanced statements). On the other hand, it will perhaps add more complication on github, as there are now many open PR and not a clear overview. What do you think? @JTurbakiewicz

@aothms aothms merged commit 2cf1295 into buildingSMART:main Sep 21, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants