feat(model): Add properties for units and tolerance #45
Conversation
chriswmackey
added
enhancement
This commit allows schema users to store the units system and tolerance with their Model JSONs. The aim of this is that, by specifying these values, schema users don't need to worry about tedious geometry checks like whether all faces point outwards from a Room. This also means that they can build the Models in the units systems that they want and our core libraries will take care of any geometry scaling needed before the export to simulation engines. This should be a relief to Konrad who has to deal with Revit using feet as its native units system. @mostaphaRoudsari , the only case that worried me a bit about storing tolerance and units systems on the Model is that I know you have a whole separate schema for point grids and this makes me wonder if the units system should be specified "one level higher" above both the Model and the point grids. But, given that Radiance doesn't expect the geometry to be in a certain units system and we don;t need to scale the model to run it through Radiance, this might be extra complexity with no real benefit. Still, I wanted to check with you about this and make sure you don't see an issue.
|
@chriswmackey thanks, does this mean I don't have require rhino user keep unit system to meter? and the Json->OSM measure would convert the units? |
| 'whether Room faces form a closed volume and subsequently correcting all ' | ||
| 'face normals point outward from the Room. Any negative values will result ' | ||
| 'in no attempt to evaluate whether the Room volumes is closed or check ' | ||
| 'face direction. So it is recommended that this always be a positive ' |
There was a problem hiding this comment.
I think the issue with that is that 0 is a real tolerance in some cases. It's an incredibly strict one that only few cases can meet without running into Python floating point issues. But I'm not sure we want to stop people from using 0 tolerance if they want. Rhino doesn't allow 0 tolerance. And it is nice to have just one value that bypasses the tolerance check and not the whole set of negative numbers. I guess my only concern is that someone sees 0 and mistakes it for a really strict tolerance check rather than no tolerance check at all. Do you think there's any possibility of this happening?
There was a problem hiding this comment.
I guess if we're clear about it in the description, using 0 will be fine. Just to be sure that I don't sow confusion, though, I think I may go back and change the places that I actually used 0 as a real tolerance in the core libraries. Some of the methods like this one should be changed so that the tolerance is required rather than having an optional 0. Or maybe we just use a different default that's no 0 like what I'm doing here:
https://github.com/ladybug-tools/honeybee-core/blob/master/honeybee/face.py#L964
There was a problem hiding this comment.
I decided to go with 0 and enforce the following rules:
- 0 tolerance indicates cases where tolerance checks should be bypassed. I'm not considering it a "real" tolerance any more.
- All honeybee-core methods requiring a tolerance input have a default value that is based on typical building geometry in meters (0.01).
- All ladybug-geometry methods requiring a tolerance will enforce that the user input the tolerance (so there's no default). I still have to implement this one completely but I've opened an issue for it: Make tolerance required on all methods that use it as an input argument ladybug-geometry#89
|
@mostaphaRoudsari . I was a little worried you would say we should add these 3 properties to the point grid schema files since having multiple places where these properties are specified means more places where we have to check that they are out of sync and come up with a logic for which object's units system/tolerance governs the other when we want to merge them. I could live with only adding a units system variable to the point grid files but, if we find ourselves in a position of adding tolerance there, I would advocate for a separate schema for |
|
Yes, the idea is that the Model geometry can be in whatever units that you want and we'll take care of conversion/scaling upon serialization. This is helpful not only for going between SI and PI but I know a lot of people want to work in Millimeters or Inches as opposed to Meters or Feet. Note that we still plan to enforce that all other properties use SI units. So, for example, the R-values of materials are always the same SI units. But geometry units in the schema can vary. |
Hopefully, we can revert the workaround once the Pydantic team either lets us know that we're using the schema wrong or pushes a fix.
chriswmackey
changed the title
|
🎉 This PR is included in version 1.14.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
This commit allows schema users to store the units system and tolerance with their Model JSONs. The aim of this is that, by specifying these values, schema users don't need to worry about tedious geometry checks like whether all faces point outwards from a Room. This also means that they can build the Models in the units systems that they want and our core libraries will take care of any geometry scaling needed before the export to simulation engines. This should be a relief to Konrad who has to deal with Revit using feet as its native units system.
@mostaphaRoudsari , the only case that worried me a bit about storing tolerance and units systems on the Model is that I know you have a whole separate schema for point grids and this makes me wonder if the units system should be specified "one level higher" above both the Model and the point grids. But, given that Radiance doesn't expect the geometry to be in a certain units system and we don;t need to scale the model to run it through Radiance, this might be extra complexity with no real benefit. Still, I wanted to check with you about this and make sure you don't see an issue.