-
Notifications
You must be signed in to change notification settings - Fork 21
IVS-236 - GRF005 - CRS unit type differences #411
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
base: development
Are you sure you want to change the base?
Conversation
features/steps/givens/values.py
Outdated
| } | ||
|
|
||
| # determine the crs unit values | ||
| crs = context.model.by_type(crs_entity_type)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A model could potentially span two coordinate zones, therefore we shouldn't assume there will always just be one relevant crs to check. Suggested revision:
coord_reference_systems = context.model.by_type(crs_entity_type)
for crs in coord_reference_systems:
...| #14=IFCSIUNIT(*,.AREAUNIT.,$,.SQUARE_FOOT.); | ||
| #15=IFCSIUNIT(*,.VOLUMEUNIT.,$,.CUBIC_FOOT.); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be valid. Imperial units need to be defined as conversion-based from their SI equivalents. Then the conversion-based length and area units get assigned via IFCUNITASSIGNMENT similar to how angular degrees # 18 is included in # 19.
ifcopenshell.api.unit may be helpful in sorting this out.
| #19=IFCUNITASSIGNMENT((#13,#14,#15,#18)); | ||
| #20=IFCPROJECT('2X9wjf5oPB4PQjenCYrhHZ',#5,'',$,$,$,$,(#11),#19) | ||
| #21=IFCPROJECTEDCRS('EPSG:2277',$,'NAD83',$,'','3',$); | ||
| #22=IFCMAPCONVERSION(#11,#21,316131.64,5690966.11,1.,1.,0.,0.3046); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EPSG:2277 identifies the length unit as US Survey Foot which is not the same as international foot (exactly 0.3048 m). In either case, 0.3046 is incorrect. In this case it should be:
Python 3.12.6 (v3.12.6:a4a2d2b0d85, Sep 6 2024, 16:08:03) [Clang 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> 1200 / 3937
0.3048006096012192Let the record state that I am merely providing information and not advocating for perpetuation of this obsolete length measure.
| #20=IFCPROJECT('2X9wjf5oPB4PQjenCYrhHZ',#5,'',$,$,$,$,(#11),#19) | ||
| #21=IFCPROJECTEDCRS('EPSG:2277',$,'NAD83',$,'','3',$); | ||
| #22=IFCMAPCONVERSION(#11,#21,316131.64,5690966.11,1.,1.,0.,0.3046); | ||
| #23=IFCGEOMETRICREPRESENTATIONCONTEXT($,'Model',3,1.E-05,#9,#10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appears to be a duplicate of # 11.
I left a comment about conversion factors and IfcConversionBasedUnit. Ultimately it should still be assigned to IfcProject via IfcUnitAssignment and therefore accessible through IfcProject.UnitsInContext.Unit. Can you point to a specific example test file(s) that does not have imperial values in |
I am leaning towards checking all three attributes that potentially refer to an EPSG code, but I can't say how often this actually happens. If there are different units for HorizonalDatum and VerticalDatum, that would indicate the need to use IfcMapConversionScaled. |
Read the docs for IfcCoordinateOperation if you have not already. IfcGeographicCRS can be used with IfcRigidOperation. |
aothms
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you have questions, quite a few comments from my side and it's a bit chaotic. Most important: focus on length and expect Scale to be the quotient of the actual factors (not based on the equivalence of name strings).
|
|
||
| Given A model with Schema 'IFC4.3' | ||
| Given An .IfcMapConversion. | ||
| Given There must be at least 1 instance(s) of .<IfcCoordinateReferenceSystem>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a bit fan of "Given There must be ..." can we rephrase this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IfcMapConversion.TargetCRS = (IfcCoordinateReferenceSystem) is also a mandatory attribute so I don't see how this could be different (or at least.. it's already handled by the schema check).
| Given A model with Schema 'IFC4.3' | ||
| Given An .IfcMapConversion. | ||
| Given There must be at least 1 instance(s) of .<IfcCoordinateReferenceSystem>. | ||
| Given The <unit> unit(s) of the project ^is not^ equal to the <unit> unit(s) of the .<IfcCoordinateReferenceSystem>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Given The <unit> unit(s) of the project ^is not^ equal to the <unit> unit(s) of the .<IfcCoordinateReferenceSystem>. | |
| Given The <length_or_angle> unit of the project ^is not^ equal to the <length_or_angle> unit of the .<IfcCoordinateReferenceSystem>. |
Maybe this reads better?
| return map | ||
|
|
||
| # determine the project unit values | ||
| project = context.model.by_type("IfcProject")[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[0] is always scary of course. Maybe we can work our way around this...
| Then .Scale. ^is not^ empty | ||
| Then .Scale. ^is not^ 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than requiring not empty and not 1.0, can we require the actual value that is needed? I.e the one conversion factor divide by the other.
| Scenario Outline: When the length unit of the Local CRS (from IfcProject) is not equal to the length unit of the Projected CRS, then the IfcMapCOnversion.Scale must be provided and cannot be 1.0 | ||
|
|
||
| Given A model with Schema 'IFC4.3' | ||
| Given An .IfcMapConversion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, MapConversion is a bit strange in the sense the SourceSRC is a SELECT between (CoordinateRef, RepresentationContext).
I think this allows us to workaround the by_type(IfcProject)[0], because we can do MapConv --Source--> RepContext -> Project/Context -> Unit.
So I think we should also condition this check to only execute when MapConv.Source is the RepContext (I'm not aware of any scenario in which this is not the case, but still).
| | length | IfcProjectedCRS | | ||
| | angle | IfcGeographicCRS | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I look at the mapconv docs I don't see any reference to scaling angular units and I don't think this actually is a thing.
NOTE The Scale can be used when the length unit for the 3 axes of the map coordinate system are not identical with the length unit established for this project (see IfcProject.UnitsInContext) - for example to convert feet into metres. If omitted, the scale factor 1.0 is assumed.
https://ifc43-docs.standards.buildingsmart.org/IFC/RELEASE/IFC4x3/HTML/lexical/IfcMapConversion.htm
So I think the rule only has to check length
| if length := crs_units.get('length'): | ||
| prefix = getattr(length, 'Prefix', '') | ||
| name = getattr(length, 'Name', '').lower() | ||
| crs_units['length'] = (f"{prefix}{name}" if prefix else name).lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of focussing on names not being equal, i'd rather focus on the conversion factors
Divide that with the unit scale reported by proj and that's the value of Scale you expect.
Co-authored-by: Thomas Krijnen <t.krijnen@gmail.com>
Co-authored-by: Thomas Krijnen <t.krijnen@gmail.com>
This rule is a bit more complex and less straightforward, some points of (potential) discussion;
IfcGeographicCRSthere is often noIfcMapConversion; do I miss something?IfcConversionBasedUnit(not through IfcProject.UnitsInContext.Unit).Is this potential for a rule? the
VerticalDatumof a CRS is described as: