Add model properties transforms

[ed-p-may]

Following up on Issue #952

I have gone in and added the transforms to the relevant items.

Passes transforms (move, scale, rotate, rotate_xy reflect) down to all extension properties of HVAC and Hot-Water objects.

• Add transform methods to:
  • model.Model
  • hvac.idealair.IdealAirSystem
  • hvac._base._HBACSystem
  • properties.extensions._EnergyProperties
  • shw.SHWSystem
• Add tests for all

[chriswmackey]

Hey @ed-p-may ,

Looking over the issue again, I'm happy to merge this if it's blocking you but I realized that I never really understood why you were assigning geometry to HVAC and SHW systems like this. I guess I just wanted to make sure that these "HVAC geometry properties" aren't better assigned similarly to all of the other geometry attributes of honeybee-energy. For example, we don't write the material thicknesses of constructions in Honeybee Model units and scale them every time the Model units change. We just say that "all material thicknesses are in meters" and we're done with it. I guess I would just appreciate knowing why this method of "assign flow rates in m3/s and duct dimension in meters" did not work for your case.

Another question I might have is, if there is real geometry here, why did you assign it to the honeybee-energy HVAC objects, which don't have any geometry associated with them to begin with? Was it just challenging to assign them as an extension of the Core Honeybee geometry objects? For example, there could have been ModelPHProperties.hvac_geometry similar to ModelRadianceProperties.sensor_grids. Things are just a little neater when geometry objects extend geometry objects. But I guess this was not something you could easily separate from the HVAC systems you wanted to use in E+?

[ed-p-may]

Hey @chriswmackey

Yes, sorry for any confusion here. Happy to try and explain, and maybe there is a cleaner and better way to do this? Also just for background: the specific case where this is popping up is users who work in IP units in Rhino - they are getting erroneous values output for the HVAC related items. The easy workaround for the time being is to always model in meters in Rhino.

---

Explanation:

So yes, the issue here I was running into was that we were trying to store some 'geometric' information for Pipes and Ducts within the HVAC Systems. The geometry we're referring to here is all Polyline3D objects which describe the center-line of the duct/pipe. This information is used later for visualizations, reporting, and calculating length parameters.

Right now, these entities are part of the HVAC and SHW 'systems' and all of the information is held on the .IdeaAirSystemPhProperties. So for instance, the user might make a duct, assign attributes to it (diameter, insulation, etc...) and then add it to a HVAC-System. An example would look something like this:

So really, what the PR here is trying to accomplish is just passing along the transform-event down to all the .properties of the HVAC and SHW systems. Once it is in the properties we handle the transform on the object. Specifically the new entities are all stored in the .properties of the HVAC and SHW objects. So for instance, ventilation exhaust ducting is held in places like:

room.properties.energy.hvac.properties.ph.ventilation_system.exhaust_ducting

---

Other Possible Implementations:

If you think adding this transform event pass-through causes a problem in the HB-Energy objects, then by all means we can see if we can find an alternative approach? I can think of a couple off the top of my head:

1. Always store duct/pipe the information as Meters, regardless of the Rhino units. This behavior would align with how things like Material thickness works, but is different than how all the other LBT-geometry works. But certainly its easy enough to convert over the user-input geometry to Meters in all cases at then time that it is input by the user? It feels inconsistent with how 'normal' geometry in HB is handled however? 2. We can add a separate function to execute the required transform on model load. This would have to be called by the user alongside (before or after) the model.scale() and there wouldn't be any really way to enforce it? But so long as the user knows that they have to do this extra step, it could work. There are not that many different place where we load in the model, so it seems conceivable that we could add this cleanup function there for this specific use-case. 3. Move this duct and pipe geometry someplace else outside the HVAC/SHW systems. We were thinking the duct/pip was part of the system, so therefor it belonged 'in' the system's properties. But perhaps we should keep this geometric data in another place in the model and just reference it into the system? I suppose we could move all the elements over to the [room.properties.ph]() which does receive the transform event already? It would certainly take some work to move things like that, but it might make sense I suppose? I does feel odd that these 'energy' related items would be outside hb-energy though?

---

I suppose my main argument in favor adding these transform event pass-throughs would be for the consistency? Meaning: that any object with .properties should always pass along events to any of its extensions. If this behavior is consistent throughout all hb-objects, that just makes things a bit easier to code against? But certianly that might not be a good enough reason to lard up the objects if you think it makes it more confusing.

@ed-p-may

[chriswmackey]

Thanks for your explanation @ed-p-may . This is all really helpful and I'm sorry that I didn't ask for more clarification before you invested time in this.

I am happy to merge this as is just so that you can get something that works for your IP users. But want to clarify that this is not really what I would recommend as a long term solution for your case. What you have in this PR isn't going to break anything as far as I can tell nor is it going to kill the performance of anything. But I don't 100% agree with this statement because following this religiously will kill the performance of all the transform operations:

I suppose my main argument in favor adding these transform event pass-throughs would be for the consistency? Meaning: that any object with .properties should always pass along events to any of its extensions.

If I were to come up with a recommendation for when it is appropriate to pass transform operations through to extensions, it should only be done in cases where there needs to be ladybug-geometry objects assigned to the extension object (like sensor grids in Radiance or the daylight sensor position in energyplus). If we were to do it for every single extension object, this means that you need to collect all of the extension objects across the model every time you just want to move the Room geometry or change the units.

Of your three alternatives, option 3 is the one that I would recommend for your case, where you assign instances of your own PH-style HVAC class directly to the Honeybee Room objects and you collect the unique instances of these PH HVACs on ModelPHProperties. Looking at your screenshots of Grasshopper components, this is what I would assume is happening under the hood and not some deeply-nested acrobatics that result in a property as long as room.properties.energy.hvac.properties.ph.ventilation_system.exhaust_ducting.

With option 3, you are doing your management of your HVAC objects separately from the the EnergyPlus HVAC object instances in honeybee-energy, which don't know about your specific honeybee-ph geometry and might not handle them correctly. As an example, when we serialize the Honeybee Model to JSON, honeybee-energy collects all of the unique HVAC object instances across the Model.rooms by building a set() of the HVAC objects, which uses the __hash__ method of the HVAC like this one that you can see here for the IdealAirSystem. These __hash__ methods have been specifically written by me to make sure that we still count two HVAC object instances with the same identifier and properties as only one system in the HBJSONs IDFs that we write. This way, we don't get duplicate objects in the HBJON or the IDF if someone just makes a copy of the HVAC Python instance and assigns that to Rooms alongside the original HVAC object instance. But these __hash__ methods are only checking that the energy properties match and they are not checking for whether you made a copy and changed your PH geometry. So your current implementation could result in loss of this geometry during the JSON serialization if the energy properties and identifiers of two HVAC object instances match.

Another reason why I would recommend extending Honeybee Rooms and the Model directly is that Rooms and Models are not going to change as they are a stable part of the schema. But HVAC systems for EnergyPlus are still very young in terms of the full life we have planned for them. I can already tell you that a lot of Pollination users are using the DetailedHVAC class to represent this systems with Ironbug. There will probably be a lot of changes to this class in the future and likely some new types of HVAC classes added. So people assigning properties directly to these HVAC instance are potentially exposed to having their workflows broken by these future changes.

So I would really recommend that you have your own ModelPHHVAC class that you assign to Honeybee Rooms like the honeybee-energy HVACs. Ad you can have your own collector property for these honeybee-ph HVAC object instances under ModelPHProperties. Then, you can handle the scaling of your HVAC geometry with your own ModelPHProperties.scale() method. Of course, you can still have methods that help you coordinate your PH HVACs with the ones in honeybee-energy. But this way, the concerns are separated and your work is not going to break because honeybee-energy is not aware that you assigned geometry to your HVACs.

What do you think

[ed-p-may]

Hey @chriswmackey ,

Ah - I see, that makes sense. Thanks for then thoughtful response.

When I wrote that original mech-equipment-code I think my hope had been that all my new PH-Mechanical-Equipment objects could integrate with the HB-Energy ones - meaning: that when the user created a 'PH' ERV or something like that in Rhino, it would also create/configure the equivalent device in the HB-Energy model. That pattern had worked well for everything else (geometry, loads, materials, etc...) up until the mechanical devices themselves. So I think this is just a legacy of that old idea.

Anyway, yes I agree: I think giving up on that notion and just moving all the PH-Mechanical-Equipment to a separate dedicated top-level plugin makes sense.

It has turned out over time that for our uses-cases we don't need to model E+ Mechanical equipment for any of our projects after all - so I think we can safely let that go and just say that if a user needs all that stuff, then use some other tool to build that part out. I think that's fine and makes sense.

So yes: I would say go ahead and close this PR without merging and we'll just take care of what we need in our own modules.

thanks! @ed-p-may

[chriswmackey]

Thank you, @ed-p-may . I'm going to close this PR, then. Thanks for the discussion and for taking the time to describe your use cases.