buildingSMART / IDS

Computer interpretable (XML) standard to define Information Delivery Specifications for BIM (mainly used for IFC)
https://www.buildingsmart.org/standards/bsi-standards/information-delivery-specification-ids/
Other
202 stars 61 forks source link

Interpretation of cardinality on specification #203

Open CBenghi opened 11 months ago

CBenghi commented 11 months ago

In the calls, the cardinality on specifications was always intended to relate to the existence of elements in the applicability set.

Prohibited, would mean that no elements should be matched, regardless from any facets in the requirements list. Required, would mean that at least one element would be matched, and then all requirements should also be matched. Optional, would mean that at if elements would match, all their requirements should also be matched.

The test cases, seem to suggest differently.

The team needs to decide which one is the most expressive approach, and document the choice made.

Moult commented 11 months ago

How does the test case contradict this?

NickNisbet commented 11 months ago

It seems extraordinary that requirements are appearing in the application section (for example) there should be a lift, instead of expressing that (for example) a building should have a lift as a requirement applicable to the building, not a lift. If the model contains two buildings , how else do I know that there should be two lifts, not one.

This valency should be deprecated as soon as possible.

andyward commented 11 months ago

How does the test case contradict this?

My reading is the test cases treat Prohibited specifications as a logical NOT of Required specifications. e.g. ids/pass-prohibited_specifications_fail_if_at_least_one_entity_passes_all_requirements_2_3.ids compared to ids/pass-required_specifications_need_at_least_one_applicable_entity_1_2.ids

With an authoring hat on, I personally think that makes most sense. Feels like people would expect Required and Prohibited to be the logical opposite of each other? This would also mean specification cardinality is aligned with Facet cardinality - e.g. attribute/fail-a_prohibited_facet_returns_the_opposite_of_a_required_facet.ids - where #144 came about

Some simple use cases to help flesh this out:

Required Use case: There must be Doors and they must have a Fire Rating property

Optional Use case: If there are Curtain Walls they must have an Acoustic Rating property

Prohibited Use case: There must NOT be any Walls where the material name matches 'unknown' (so I can undertake LCA etc)

If we treat Prohibited as disregarding all Requirement facets that last example just expresses 'There must be no walls' - and the use case intent seems more unintuitive to express. (although presumably you could use an Optional spec applying to Walls with a Prohibited material Facet)

Also see my suggestion we define some more applicability test cases as I still think there's ambiguity on the applicability side of things #181

Moult commented 11 months ago

My reading is the test cases treat Prohibited specifications as a logical NOT of Required specifications.

@andyward agreed.

Prohibited, would mean that no elements should be matched, regardless from any facets in the requirements list.

@CBenghi upon re-reading this, I realise I now disagree with this. The facets in the requirements intuitive to me at least should matter. I agree with @andyward 's interpretation which is how I ended up with the tests.

aothms commented 11 months ago

The thing with this is that it makes these two things equivalent:

and

Which is not necessarily a problem, but it might be good to nudge people into a consistent direction using the validator. I prefer the 2nd, i.e to leave requirement empty.

TLiebich commented 11 months ago

Hi @aothms and all,

when reading the comments, I assume the following basic checking logic

for all specification
  pass = 0
  if (applicability) then
    if (requirements) then
       pass+
  if prohibited AND pass = 0 return true
  if optional AND pass >= 0 return true
  if required AND pass > 0 return true

if the specification is "Doors must have a Fire Rating property" then pass=0 means either, there are no Doors, or there are doors, but none has fire rating. pass>0 means, there is at least one door (probably many) and at least one of those doors has fire rating

but pass= would mean, all doors have fire rating -> this actually meets the specification "(all) doors must have a fire rating property" -- and to me, this is not included in the prohibited, optional, required syntax, but in normal LOIN understanding the most natural validation rule

comming back to @andyward

"Required Use case: There must be Doors and they must have a Fire Rating property" what about "All doors must have fire rating property" ?

or am I miss something? hornestly - this minOccurs / maxOccurs confuses me

aothms commented 11 months ago

I see your point. This min/maxOccurs thinking seems to steer you towards counting only the passing cases in your pseudo code, but this is not enough. Because:

what about "All doors must have fire rating property" ?

Is actually still the default behaviour (i.e optional).

Consider this augmented pseudocode.

for all specification
  pass = 0
  fail = 0
  for all instances as inst
    if applicability(inst) then
      if requirements(inst) then
         pass += 1
      else
         fail += 1
  if prohibited AND pass = 0 return true
  if required AND fail = 0 AND pass > 0 return true
  if optional return fail = 0

Optional doesn't mean the fulfilling the requirements is optional. It means that the existence of passing instances in the model is optional.

Which - now that I'm writing this down - tbh sounds more confusing to me than earlier.

CBenghi commented 11 months ago

It means that the existence of passing instances in the model is optional.

In the calls we agreed it means that the existence of elements matching applicability is optional. But if they exist, requirements must be fulfilled.

for all specification
  selection = ifc instances where applicability(inst) = true
  if prohibited and count selection > 0 return fail
  if required and count selection = 0 return fail
  for all selection as inst
    if not requirements(inst) return fail
return pass

Edit: Added pseudocode

NickNisbet commented 11 months ago

Leaving the requirement empty is not natural. It is not understandable. Requirements (even if cardinality or valency) should not be in applicability . ‘There should be a lift’ is not a proper requirement. It is ’a building that should have a lift’.

aothms commented 11 months ago

@NickNisbet your example is more of a design requirement instead of a data specification requirement. The required/prohibited existence of a specification IDS is intended to require or disallow certain technical modelling conventions: e.g

or to broadly indicate what types of elements you expect for example if you're a manufacturer or subcontractor.

In those cases I find the distinction you're making less relevant. But I agree it is something that needs to be reconsidered for future versions of IDS.

@CBenghi

It means that the existence of passing instances in the model is optional.

In the calls we agreed it means that the existence of elements matching applicability is optional. But if they exist, requirements must be fulfilled.

Confirmed. That is indeed what I was trying to say, but probably again not clear enough.

andyward commented 11 months ago

Really feels like we need to clarify which of these algorithms spec cardinality should follow...

  1. Spec Cardinality defines the expected number of matches from the applicability facets - regardless whether the applicable selection meets the requirements

    for all specification
    selection = ifc instances where applicability(inst) = true
    if prohibited and count selection > 0 return fail
    if required and count selection = 0 return fail
    for all selection as inst
    if not requirements(inst) return fail
    return pass
  2. Spec Cardinality defines the expected results of checking applicable items against the requirements

for all specification
  pass = 0
  fail = 0
  for all instances as inst
    if applicability(inst) then
      if requirements(inst) then
         pass += 1
      else
         fail += 1
  if prohibited AND pass = 0 return true
  if required AND fail = 0 AND pass > 0 return true
  if optional return fail = 0 

According to what is in current IDS documentation the intention is it's the second algorithm:

Each Specification may also specify whether it is Required, Optional, or Prohibited. Given the example Specification of "all walls must have a fire rating property" this is the interpretation:

Type Meaning Example
Required The specified information must be found in the IFC model The model must have walls, and they must all have a fire rating property
Optional If there are elements in the IFC model that are applicable to the Specification, then the Requirements must be satisfied The model may or may not have walls. If walls exist, then they must have a fire rating property
Prohibited The specified information must not be found in the IFC model The model should not have any walls that have a fire rating property. Walls without a fire rating property are allowed. Other non-wall elements with a fire rating property are also allowed.

If that's what's documented and in the test cases, sounds like an easy decision, unless we want to re-think?

@aothms good point about the potential ambiguity.

it might be good to nudge people into a consistent direction using the validator

Perhaps the IDS validator should warn when using a non-favoured approach. I agree about Nick's point about omitting the requirement feeling un-natural - and warning when there's no requirement feels more logical than the alternative. That said, there will often be multiple different ways of expressing the same requirements - adherence to the test cases is probably as important as a spec validator if we're to achieve portability between implementations?

@NickNisbet I'd suggest we move discussion of the requirements like 'Buildings have a lift' etc to a separate topic as it feels like a different issue to spec cardinality (and related to your RASE thinking - Applicability vs Selection etc)? Pretty sure current IDS supports that requirement via PartOf though

NickNisbet commented 11 months ago

Yes, my example was a bit too functional, but the point that requirements are appearing in the application section stands. This lack of clarity is happening because of the lack of expressiveness of ids1. Applying RASE to the three explanatory sentences makes this clear: The three cases apply to the model, not to a wall. Always ask ‘what fails?’ ‘Quid defecit?’

Sent whilst away from my desk.

Regards,

Nick.

CBenghi commented 11 months ago

@andyward

If that's what's documented and in the test cases, sounds like an easy decision, unless we want to re-think?

The reason documentation and test cases is because they were both written by @Moult, but that differs from the intent that emerged from the analysis of the use cases.

The analysis of use cases suggests that BIM managers tend to distinguish various model subsets (e.g. Internal walls, external walls, classified elements, etc), and then apply requirements to that set.

Cardinality of specs if the first type of (prohibited, required and optional), just on the set.

Then you have information expectations (requirements). Those define the specific data structures that are expected or prohibited on the subset.

I would argue that the example given: The model should not have any walls that have a fire rating property. is much more intuitively expressed with an identification of the subset of the model (applicability: walls) and one explicit requirement (fire rating property is prohibited).

In this way all expectations around the models can be nicely arranged in tidy conceptual breakdowns of model subsets (that can be reused across projects) and their requirements.

While from an IT implementer the difference is merely formal, from an UX and ecosystem perspective I much prefer this view.

aothms commented 11 months ago

I see your point @CBenghi. It does indeed make sense to be able to change the occurrence constraints on a specification without having to move around items between applicability and requirement. I would propose to add a note though in the documentation with an example that these two statements (specifically in the case of prohibited) are logically equivalent https://github.com/buildingSMART/IDS/issues/203#issuecomment-1758037864 Because I still believe it's a bit of an astonishing fact.

andyward commented 11 months ago

Totally get the use case, @CBenghi and indeed have both seen and built EIR requirements where applicability and requirement are split, and linked together via a relationship.

If you're changing the logic so we only use it to check the applicable items, would it make more logical sense (and make the intent clearer) to then move this cardinality from the specification to the applicability node?

<ids:specification name="There must be a building" minOccurs="1" maxOccurs="unbounded">
  <ids:applicability>
    <ids:entity>
      <ids:name>
        <ids:simpleValue>IFCBUILDING</ids:simpleValue>
      </ids:name>
    </ids:entity>
  </ids:applicability>

becomes

<ids:specification name="There must be a building">
  <ids:applicability minOccurs="1" maxOccurs="unbounded">
    <ids:entity>
      <ids:name>
        <ids:simpleValue>IFCBUILDING</ids:simpleValue>
      </ids:name>
    </ids:entity>
  </ids:applicability>

That would make the intent clearer in my view.

CBenghi commented 11 months ago

Hi @andyward, You have a point on the location of the attributes. We could leave them there for simplicity, or move them for expressiveness.

MatthiasWeise commented 11 months ago

Good discussion! I am in favour of decoupling occurrence of instances from our requirement statements, even if we loose a little bit of expressiveness. So, I agree with the interpretation from Claudio and 1+ for the proposal from @andyward for moving occurrence to applicability.

This specification:

    Applicability Cardinality: PROHIBITED
        IFCWALL entity
        Material Value matching pattern '.*unknown.*'
    Requirement: -

could then be translated into:

    Applicability Cardinality: OPTIONAL
        IFCWALL entity
    Requirement: PROHIBITED 
        Material Value matching pattern '.*unknown.*'

Requirement: PROHIBITED is however questionable for properties due to the datatype issue described in #206.

From an engineering point of view the specification might be even more like this:

Either (all IFCWALL instances must have material != unknown")

    Applicability Cardinality: OPTIONAL
        IFCWALL entity
    Requirement: REQUIRED
        Material 

    Applicability Cardinality: PROHIBITED
        IFCWALL entity
        Material Value matching pattern '.*unknown.*'
    Requirement: -

Or (with exclusion statement in our material pattern).

    Applicability Cardinality: OPTIONAL
        IFCWALL entity
    Requirement: REQUIRED 
    Material Value matching pattern '^(?!.*unknown).*$'
aothms commented 11 months ago

exclusion statement in our material pattern

Note that we have agreed not to allow this https://github.com/buildingSMART/IDS/issues/29#issuecomment-1040245815

MatthiasWeise commented 11 months ago

Note that we have agreed not to allow this #29 (comment)

Good point, keep it simple! Then we need to split into two specifications (first option).

CBenghi commented 11 months ago

Decision: cardinality is related to applicability only. We will:

@CBenghi will start a PR, then we can improve it until it's completed.

TLiebich commented 11 months ago

@CBenghi and all,

if we do such a last minute schema change now (and have to rework examples, docs and prototype implementations anyway), wouldn't it be the best (and last possible) time to rename the minOccurs and maxOccurs to avoid future missunderstandings?

CBenghi commented 11 months ago

@TLiebich, that was also my proposal in the call, but there was some resistance from the implementers. If you want to join the call tomorrow to champion this view, you are very welcome!

An objection was also raised that it might come in handy at some point, but I still think we should have our own (more sophisticated) way of expressing cardinality, and min/max was never a good idea.

MatthiasWeise commented 11 months ago

Changing the namespace from xs to ids sounds a good compromise if we want to keep cardinality for future IDS versions (although I agree that renaming shouldn't be a deal breaker either). But again, even if we will not support cardinality in IDS 1.0 and limit to required, optional and prohibited we should be clear about future meaning of min/max for both applicability and requirements. While it is clear for applicability now, it remains unprecise for requirement types (property, partOf, classification, material).

berlotti commented 11 months ago

Better to have a painful change now (while the standard is not final yet); than a deepening wound later... Dare to do the right thing!

TLiebich commented 11 months ago

Hi all, sorry for not being able to join the call (away on holidays), but I would also emphasize on "do the right thing at the right time". And to me "right time" is now (later it will be far more difficult).

CBenghi commented 11 months ago

On balance, we have agreed to keep min and max occurs for applicability

Pros:

Cons:

We have also extended the discussion to include cardinality of requirement facets.

In this case it was agreed that we will NEVER need to use min/max cardinality (e.g. in the context of property, classification and such), so we decided to replace with an enumeration.

The proposed enumeration was only limited to Required, and Prohibited, because the Optional case was deemed difficult to understand and implement, and the team from Acca noted that the same could be achieved by creating an extra specification attaching value constraint to any entities matching the property (the conversation was mostly concerned with values).

This is were the call was left, however - upon implementing the changes - I have noticed that the Optional case achieves an extra data requirement that we have not considered in the call: it provides a convenient way to define the the datatype of a non-standard property, and its association to a model subset (i.e. applicability) in one location.

While this might be more verbose way of doing it (it might require redefining the same property for multiple model parts), it can be argued that it is easy to understand for BIM managers.

Case in point the OMA development file does something like this:

<ids:property minOccurs="1" maxOccurs="unbounded" datatype="IFCLABEL" instructions="Primary travel direction of people in a given area. For example, the flow direction of people in a one-way corridor.">
    <ids:propertySet>
        <ids:simpleValue>ePset_SpaceOMA</ids:simpleValue>
    </ids:propertySet>
    <ids:name>
        <ids:simpleValue>TravelDirection</ids:simpleValue>
    </ids:name>
</ids:property>
<ids:property minOccurs="0" maxOccurs="unbounded" datatype="IFCREAL" instructions="The maximum flow rate which the device is designed, theoretical or expected to achieve.">
    <ids:propertySet>
        <ids:simpleValue>Pset_SpaceOccupancyRequirements</ids:simpleValue>
    </ids:propertySet>
    <ids:name>
        <ids:simpleValue>DesignFlowRate</ids:simpleValue>
    </ids:name>
</ids:property>

The firs property is clear, it defines a pset and prop name along with its datatype and it is required! The second one also defines a pset and prop name along with its datatype, and makes it optional.

While I personally don't see why people would bother defining a property when it is not needed. We need to discuss if the Optional enum needs to be added to the xsd, and if it makes sense to keep it constrained to the property facet or expand it to others.

I will update the PR soon, and I've also been working on an updated audit tool.

MarcelStepien commented 10 months ago

Just to add my option on the matter of the cardinality in regard to to required and prohibited, I agree that an IDS specific solution would solve a lot of issues regarding the currently missleading and borrowed nature of minOccurs and maxOccurs (since its XML-Schema specific, not conceived to be used for rule checking). An enumeration that can maybe in later versions be extended would do just fine.

@CBenghi In regards to adding Optional to the enum, in my opinion there can exist information that should be checked if provided and should be ignored if not (making it optional). Assuming that IDS is not the last barrier of quality assurance in a lifecycle of a model, the benefit is that subsequent algorithms, tasks and project phases may decide to incorporate or exclude certain processes based on those optional checked parameters.