-
Notifications
You must be signed in to change notification settings - Fork 61
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
Remove cardinality checks for attributes #144
Comments
As per Pasi's proposal on buildingSMART#144.
If @pasi-paasiala 's proposal is agreeable, which I support, just merge PR #149. |
Why is the interpretation vague? There are clear examples here https://github.com/buildingSMART/IDS/blob/master/Documentation/testcases-ids.md and the meaning seems clear to me https://github.com/buildingSMART/IDS/blob/master/Documentation/developer-guide.md#optionality |
Vagueness
Does an optional requirement on an optional attribute specify anything beyond what is already defined in the IFC schema itself? Can it be safely ignored without worry to invalidate the result of a check? What if the value is not Foobar as defined in the requirement, but Barfoo? Should that fail the check? Examples All of the referenced examples use: which is not according to the referenced specification: What was the originating intention? I believe these should be addressed in #148 (or other PR) as well. |
I try to keep the development files up to date with the schema. |
The testcases need updating, but I think the intention is spelled out already: Also notably, the occurs can be used to set prohibited. So you can say "This entity shall not have this attribute filled out". |
Presumably the integration test case data using attribute facets also need amending? i.e those IDS files at https://github.com/buildingSMART/IDS/tree/master/Documentation/testcases/attribute |
I don't think we encountered this requirement in any of the use cases. |
The discussion on prohibited came fairly late. I can't reconstruct it entirely either, but I think it makes sense to be consistent here. If we want to enable forbidding certain entity usage, certain property usage, we can also prohibit attribute usage. It makes sense that the use cases we have focus on what they need and not what they do not need. But the act of forbidding a certain attribute constructs can be very informative, such as: "IfcSite.RefLatitude should not be used, rather use IfcMapConversion". So for me, setting maxoccurs=0 (prohibited) or minoccurs=1 (required) on an optional attribute makes sense. facet specification with minoccurs=0 (optional) is a general recurring topic, we apparently had the desire to make optional statements: "if the tool has this data please provide it here". For me this warrants maintaining the attribute occurs constraint as we have it, but I agree there is more effort needed in spelling out all the implications, because I agree the majority of permutations of attribute types and occurrence constraints are pure noise. But that's the consequence of the choice we made to model prohibited/optional using integers. |
TLDR: We need to discuss whether to bring back Long: Your argument makes sense to me. We might have jumped the gun writing the PR.
Indeed... the original sin! ;-) It would be nice to resume a regular meeting to bring IDS to a robust 1.0 in the medium short time-frame. It could be the right venue to come to sound decisions on the closure of issues and merging of PRs. |
Any updates on this? |
Commented out tests involving minOccurs and maxOccurs until buildingSMART/IDS#144 gets resolved.
Can we please bring back the attribute cardinality? I really don't get the logic for this. There are clear test cases and documentation describing how it is interpreted. |
The preliminary agreement on the call was as follows: Document that cardinality applies to the value, not the existence of the attribute itself in the model. However, the conversation moved to a bigger issue of the interpretation of the cardinality on specification, that we need to close before we make any progress here. See #203 |
Changed to an expressive enum, where alreadu applicable. Type does not allow for prohibitions, as we prefer to use enumeration to restrain the allowed values. Cardinality of attributes is to be discussed in a separate issue buildingSMART#144
Added to the schema changes presented in the call on 2024-02-02 |
* Improving clarify of datatype and Property facet - The schema is changed to be more expressive with capitalization - The documentation better expresses the capitalization of datatypes - Improved units page * Working on #203 - [x] improved documentation - [x] change the schema to move MinMaxOccurs to applicability - [x] update audit tool - [ ] correct test cases appropriately * Added OMA back with fixes. * Updated tooling * Updated tooling * Improved specification documentation * Changed the cardinality of requirement facets Changed to an expressive enum, where alreadu applicable. Type does not allow for prohibitions, as we prefer to use enumeration to restrain the allowed values. Cardinality of attributes is to be discussed in a separate issue #144 * Modified development files per call agreement We have still to discuss Optional cardinality for facets. OMA uses it for classification and property. * Further work on the specification document page. * Documentation proposal for all facet configuratitions. * All schema changes previously discussed. * Further fixes discussed in the call today. * Revised Development files. * Update Development/IDS_random_example.ids Spelling fixed. Co-authored-by: Stefan Jaud <59165496+pjanck@users.noreply.github.com> * Schema change, cardinality made CamelCase. * doc changes. * Added testcases generation and improved schema Improved documentation in the XSD file with documentation and minor consistency changes. Updated all testcases to latest schema. Ids testcases are generated from the documentation/testcases/scripts.md via the SchemaProject folder. The next step is to integrate this feature in the automated targets * Fixed IDS files - completed information on Development files - documentation: dataType adjusted to valid scenarios - testcases adjusted for audit: - attribute/pass-an_optional_facet_always_passes_regardless_of_outcome removed - improved empty classification systems (requiring at least a letter). - improved entity constraints - improved material tests - improved partof tests * All testcases pass IDS auditing deliberately A new class of tests (invalid) has been added, to mark tests that should fail IFC verification and and are also invalid for the audit tool. * Completed the unit documentation. * Schema changes from SCTE consultation - propertyType.name renamed to baseName - EntityType.subType renamed to predefinedType - Improved documentation * Corrected some test cases. * Improved units.md - removed units that never require conversion - corrected unit capitalization according to SI - consistent spacing in Unit symbols --------- Co-authored-by: Stefan Jaud <59165496+pjanck@users.noreply.github.com>
* Preparing development branch the next version of the schema is prepared, upon which every schema change will be developed. Only schema versions merged int the main branch will stay unchanged. Schemas in development can be modified until merged into main. * Prepared change to allow multiple material facets See #211 * Fix: maxOccurs of restriction within idsValue The value has been limited to 1, this was a mistake in the schema definition, it was alsways interpreted as 1 by the implementers. * Facets review (#240) * Improving clarify of datatype and Property facet - The schema is changed to be more expressive with capitalization - The documentation better expresses the capitalization of datatypes - Improved units page * Working on #203 - [x] improved documentation - [x] change the schema to move MinMaxOccurs to applicability - [x] update audit tool - [ ] correct test cases appropriately * Added OMA back with fixes. * Updated tooling * Updated tooling * Improved specification documentation * Changed the cardinality of requirement facets Changed to an expressive enum, where alreadu applicable. Type does not allow for prohibitions, as we prefer to use enumeration to restrain the allowed values. Cardinality of attributes is to be discussed in a separate issue #144 * Modified development files per call agreement We have still to discuss Optional cardinality for facets. OMA uses it for classification and property. * Further work on the specification document page. * Documentation proposal for all facet configuratitions. * All schema changes previously discussed. * Further fixes discussed in the call today. * Revised Development files. * Update Development/IDS_random_example.ids Spelling fixed. Co-authored-by: Stefan Jaud <59165496+pjanck@users.noreply.github.com> * Schema change, cardinality made CamelCase. * doc changes. * Added testcases generation and improved schema Improved documentation in the XSD file with documentation and minor consistency changes. Updated all testcases to latest schema. Ids testcases are generated from the documentation/testcases/scripts.md via the SchemaProject folder. The next step is to integrate this feature in the automated targets * Fixed IDS files - completed information on Development files - documentation: dataType adjusted to valid scenarios - testcases adjusted for audit: - attribute/pass-an_optional_facet_always_passes_regardless_of_outcome removed - improved empty classification systems (requiring at least a letter). - improved entity constraints - improved material tests - improved partof tests * All testcases pass IDS auditing deliberately A new class of tests (invalid) has been added, to mark tests that should fail IFC verification and and are also invalid for the audit tool. * Completed the unit documentation. * Schema changes from SCTE consultation - propertyType.name renamed to baseName - EntityType.subType renamed to predefinedType - Improved documentation * Corrected some test cases. * Improved units.md - removed units that never require conversion - corrected unit capitalization according to SI - consistent spacing in Unit symbols --------- Co-authored-by: Stefan Jaud <59165496+pjanck@users.noreply.github.com> --------- Co-authored-by: Claudio Benghi <claudio.benghi@gmail.com> Co-authored-by: pasi-paasiala <pasi.paasiala@solibri.com> Co-authored-by: Stefan Jaud <59165496+pjanck@users.noreply.github.com>
It is possible to specify cardinality for attributes as shown in this example.
The specific requirement I'm talking about is this:
The interpretation of this is vague. I'd remove the possibility to specify cardinality for attributes. If IDS says something about an attribute, it is a requirement, not optional.
The text was updated successfully, but these errors were encountered: