Skip to content

ifctester: PartOf false pass on USERDEFINED predefinedType - #9203

Open
BIMvoice wants to merge 4 commits into
IfcOpenShell:v0.8.0from
BIMvoice:fix/ifctester-partof-userdefined
Open

ifctester: PartOf false pass on USERDEFINED predefinedType#9203
BIMvoice wants to merge 4 commits into
IfcOpenShell:v0.8.0from
BIMvoice:fix/ifctester-partof-userdefined

Conversation

@BIMvoice

@BIMvoice BIMvoice commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

PartOf never recognises predefinedType="USERDEFINED", in any of its six relation branches. The result is a false pass: a model that violates a prohibition is reported as compliant.

The cause

ifcopenshell.util.element.get_predefined_type() never returns the literal string "USERDEFINED". Its own docstring says so: it substitutes the custom ObjectType / ElementType / ProcessType text instead.

Entity.__call__ already special-cases this. PartOf.__call__ does not, at any of its six sites: aggregates, group, contained-in-spatial-structure, nests, voids and fills, and the no-relation ancestor walk.

The wrong verdict, reproduced

IFC4, an IfcSpace containing an IfcWall, checked with PartOf(predefinedType="USERDEFINED", cardinality="prohibited"):

  • Before: PASS, is_pass=True, reason PROHIBITED.
  • Correct: FAIL. The wall genuinely sits in a userdefined-type space, which the prohibition forbids.

The mirror case, cardinality="required", gives the opposite wrong answer: FAIL where it should PASS.

All six branches were built and run independently, before and after.

Why this matters more than a crash

A false pass in an IDS checker tells someone their model is compliant when it is not, and nothing surfaces the mistake later. That is why this was prioritised over the false-fail cases in the same audit.

Fix and tests

Adds PartOf.predefined_type_matches(), mirroring the pattern Entity.__call__ already uses, applied at all six call sites.

37/37 pass, both at baseline and after. Two previously untested PartOf branches gained coverage as a byproduct.

Deliberately not changed

  • Specification.minOccurs / maxOccurs as exact numeric bounds. This reads as ternary-by-design and no fixture uses other values, so changing it would be a guess about intent rather than a fix.
  • The remaining wrong-verdict site in the reporter layer is exactly what ifctester: fix four wrong-verdict defects #9190 already fixes.

Generated with the assistance of an AI coding tool.

BIMvoice added 2 commits July 31, 2026 20:09
@BIMvoice
ifctester: fix PartOf USERDEFINED false pass
cb5742e 
get_predefined_type() never returns the literal string "USERDEFINED";
for a userdefined element it substitutes the custom ObjectType (or
ElementType/ProcessType) text instead, per its own docstring. Every
PartOf relation branch compared that text directly against
self.predefinedType, so a requirement for predefinedType="USERDEFINED"
could never match. Combined with the prohibited-cardinality flip at
the end of __call__, this produced a false PASS: an element genuinely
contained in, aggregated into, grouped with, nested under, or
voided/filled by a userdefined-type parent was reported as satisfying
a "must not" requirement. The required-cardinality case produced the
mirror false FAIL.

Entity.__call__ already special-cases this via is_userdefined_type().
Added the same handling to PartOf via a shared
predefined_type_matches() helper, applied at all 6 relation branches
(including the no-relation ancestor walk). Reproduced and verified
each of the 6 branches directly against real API-built IFC4 models
before and after the fix.

Two of the six branches (no-relation ancestor walk, voids/fills) had
no prior test coverage at all; added baseline coverage alongside the
USERDEFINED regression tests.

Generated with the assistance of an AI coding tool.
@BIMvoice
docs: add ifctester verdict audit notes
0f2b2ed 
Records the branch enumeration, existing-work check against IfcOpenShell#9187-
IfcOpenShell#9190/IfcOpenShell#9058/IfcOpenShell#9059/IfcOpenShell#9142/IfcOpenShell#8407/IfcOpenShell#8292/IfcOpenShell#8253/IfcOpenShell#8161, the PartOf
USERDEFINED false-pass reproduction and fix, and the areas checked
and not pursued (bounded minOccurs/maxOccurs, reporter layer).

Generated with the assistance of an AI coding tool.
@aothms

aothms commented Aug 1, 2026 via email

Copy link
Copy Markdown
Member

@BIMvoice
ifctester: add fixture pair for PartOf USERDEFINED
5ccf00b 
Minimal .ids/.ifc pair demonstrating the false pass fixed by
this branch: a wall inside a user-defined-type IfcSpace, checked
with a prohibited PartOf(predefinedType="USERDEFINED"). Verified
red (PASS) against the pre-fix code and green (FAIL) against
this branch's fix, both through ids.open/ifcopenshell.open, not
by calling the facet directly. Requested by aothms on PR IfcOpenShell#9203.

Generated with the assistance of an AI coding tool.
BIMvoice added a commit to BIMvoice/IfcOpenShell that referenced this pull request Aug 1, 2026
@BIMvoice
ifctester: add fixture pair for prohibited req
7db4f7c 
Minimal .ids/.ifc pair: a prohibited specification (no walls
allowed) violated by one wall. Requirement checks are skipped
for prohibited specifications, so requirement.failures stays
empty; the requirement's status must still be False. Verified
red (requirement.status True) against the pre-fix code and
green (False) against this branch's fix, through ids.open and
ifcopenshell.open, not by calling the facet directly. Requested
by aothms on PR IfcOpenShell#9203/IfcOpenShell#9205 (applies repo-wide to all ids PRs).

Generated with the assistance of an AI coding tool.
BIMvoice added a commit to BIMvoice/IfcOpenShell that referenced this pull request Aug 1, 2026
@BIMvoice
ifctester: add fixture pair for table dataType
dbccf2d 
Minimal .ids/.ifc pair: an IfcWall with an IfcPropertyTableValue
carrying a real value, checked by a Property requirement with no
dataType constraint. With nothing to match against, presence
alone should pass. Verified red (FAIL, "does not match the
required data type of None") against the pre-fix code and green
(PASS) against this branch's fix, through ids.open and
ifcopenshell.open, not by calling the facet directly. Requested
by aothms on PR IfcOpenShell#9203/IfcOpenShell#9205 (applies repo-wide to all ids PRs).

Generated with the assistance of an AI coding tool.
BIMvoice added a commit to BIMvoice/IfcOpenShell that referenced this pull request Aug 1, 2026
@BIMvoice
ifctester: add fixture pair for material crash
0a02e19 
Minimal .ids/.ifc pair: a wall with an IfcMaterialLayerSet whose
single layer has no Material assigned (a valid air gap), checked
by a Material value requirement. Verified red (pre-fix code
raises AttributeError: 'NoneType' object has no attribute 'Name'
via ids.py:306, aborting the whole run) and green (post-fix, the
run completes and correctly reports FAIL since the layer has no
matching material name). Verified through ids.open and
ifcopenshell.open, not by calling the facet directly. Requested
by aothms on PR IfcOpenShell#9203/IfcOpenShell#9205 (applies repo-wide to all ids PRs).

Generated with the assistance of an AI coding tool.
@BIMvoice

BIMvoice commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Done, and applied to the other five IDS PRs as well (#9205, #9190, #9189, #9188, #9187).

src/ifctester/test/fixtures/partof_userdefined/ holds the .ids and .ifc pair, wired into test_fixtures.py which goes through ids.open(), ifcopenshell.open() and Ids.validate(), so it exercises the same path as the CLI rather than calling a facet directly.

Each pair was checked in both directions, since a fixture that passes before and after proves nothing:

  • Unfixed (a11ebdf8c4): [PASS] (1/1)
  • Fixed: [FAIL] (0/1)

There were no .ids/.ifc fixtures in src/ifctester before this, so the layout is new. I followed src/ifcopenshell-python/test/fixtures/, which test_validate.py and test_mvd_info.py already use. Happy to move it if you would rather it sat elsewhere.

This was referenced Aug 1, 2026
Open
Draft
BIMvoice added a commit to BIMvoice/IfcOpenShell that referenced this pull request Aug 4, 2026
@BIMvoice
ifctester: add fixture for bounded restriction
0d460c7 
Minimal .ids/.ifc pair: an IfcWall with an IfcPropertyBoundedValue
carrying only an UpperBoundValue of 40, checked by a Property
requirement with a minInclusive=20 restriction. The missing lower
bound could be anything, including below 20, so the requirement
must fail rather than pass by omission.

Verified red (PASS) against the pre-fix code and green (FAIL)
against this branch's fix, through the ifctester CLI (ids.open,
ifcopenshell.open), not by calling the facet directly. Requested
by aothms on PR IfcOpenShell#9203/IfcOpenShell#9205 (applies repo-wide to all ids PRs).

Generated with the assistance of an AI coding tool.
BIMvoice added a commit to BIMvoice/IfcOpenShell that referenced this pull request Aug 4, 2026
@BIMvoice
ifctester: rename to per-fixture test file
7e6cde6 
test_fixtures.py is shared with our other open ids-fixture PRs
(IfcOpenShell#9203, IfcOpenShell#9205), which each independently create the same file and
collide with each other and with this branch. Renaming to a
fixture-specific file name avoids the clash; fixture folders under
test/fixtures/ never collide since each PR uses its own subfolder.

Generated with the assistance of an AI coding tool.
@BIMvoice
ifctester: rename to per-fixture test file
405d1ac 
test_fixtures.py is shared with our other open ids-fixture PRs
(IfcOpenShell#371-branch, IfcOpenShell#9205), which each independently create the same file
and collide with each other and with this branch. Renaming to a
fixture-specific file name avoids the clash; fixture folders under
test/fixtures/ never collide since each PR uses its own subfolder.

Generated with the assistance of an AI coding tool.
BIMvoice added a commit to BIMvoice/IfcOpenShell that referenced this pull request Aug 4, 2026
@BIMvoice
ifctester: split shared test file per fixture
e5f693f 
test_fixtures.py is shared with our other open ids-fixture PRs
(IfcOpenShell#371-branch, IfcOpenShell#9203), which each independently create the same file
and collide with each other and with this branch. Splitting into
one file per fixture avoids the clash; fixture folders under
test/fixtures/ never collide since each PR uses its own subfolder.

Generated with the assistance of an AI coding tool.
This was referenced Aug 4, 2026
Open
Open
Open
Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@BIMvoice @aothms