Fix #4786 - Space load-based actuator for spaces are named based on thermal zone name #4828

jmarrec · 2023-03-15T18:34:16Z

Pull request overview

Fixes Space load-based actuator for spaces are named based on thermal zone name #4786

@mdahlhausen @lymereJ would appreciate a review / testing from you please.
@brianlball I'm tagging you as a reviewer as you're the most familiar with this piece of code, but if you can't review I can ask someone else (@tijcolem for eg)

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

…ore a Space or a ThermalZone in it.

…ce (and vice versa)

…cts, sort them by name, so we're deterministic

src/model/EnergyManagementSystemActuator.hpp

jmarrec · 2023-03-15T18:34:46Z

resources/model/OpenStudio.idd

+  A6; \field Zone or Space Name
+       \note This field is useful for a SpaceLoadInstance that is assigned to a SpaceType
+       \note because internally E+ will duplicate the instances and you need to access the final name
+       \note If you are using the Space Translation of the ForwardTranslator (which is defaulted on) this should be set to a Space
+       \note If not then use a ThermalZone
       \type object-list
-       \object-list AllObjects
+       \object-list SpaceNames
+       \object-list ThermalZoneNames


Constrain this field in the IDD. It will now only accept a Thermal Zone or a Space.

src/model/EnergyManagementSystemActuator.hpp

jmarrec · 2023-03-15T18:36:06Z

src/model/EnergyManagementSystemActuator.hpp

@@ -85,13 +94,17 @@ namespace model {
    /** @name Getters */
    //@{

-    boost::optional<ModelObject> actuatedComponent() const;
+    boost::optional<ModelObject> actuatedComponent() const;  // TODO: this should NOT be an optional


I didn't want to break API, but this should never have been an optional. Though I suppose the idea is that there's a chance the object it referenced would be deleted, right?

Thats right. what happens if that object gets removed?

You just don't translate it during FT. That can mean having the FT calling an Impl function or in a try catch
In an ideal world, removing à modelobject would mean removing any ems actuator attached, but that's probably a major slow down and we don't want to penalize anyone not using ems

src/model/EnergyManagementSystemActuator.hpp

jmarrec · 2023-03-15T18:53:23Z

src/energyplus/ForwardTranslator/ForwardTranslateEnergyManagementSystemActuator.cpp

+        // Not translating Spaces
+        std::set<std::string> zoneNames;
+        for (const auto& space : spaceType_->spaces()) {
+          if (auto tz_ = space.thermalZone()) {
+            zoneNames.insert(tz_->nameString());
+          }
+        }
+        if (zoneNames.empty()) {
+          LOG(Error, "Actuator '" << modelObject.nameString() << "' references a SpaceLoad '" << load_->nameString()
+                                  << "' attached to the SpaceType '" << spaceType_->nameString()
+                                  << "' but the Space Type has zero spaces attached to any Thermal Zones, so it will not be translated.");
+          return boost::none;
+        }
+        zoneOrSpaceName = *(zoneNames.begin());
+        if (zoneNames.size() == 1) {
+          LOG(Warn, "Actuator '" << modelObject.nameString() << "' references a SpaceLoad '" << load_->nameString() << "' attached to the SpaceType '"
+                                 << spaceType_->nameString()
+                                 << "' but you have turned off ForwardTranslation's Space Feature. Falling back to using the attached ThermalZone '"
+                                 << zoneOrSpaceName << "' since there is only 1.");
+        } else {  // if (zoneNames.size() > 1) {
+          // TODO: do we grab the first space or just don't translate it?
+          LOG(Error, "Actuator '" << modelObject.nameString() << "' references a SpaceLoad '" << load_->nameString()
+                                  << "' attached to the SpaceType '" << spaceType_->nameString()
+                                  << "' but you have turned off ForwardTranslation's Space Feature. "
+                                     "Your SpaceType ends up linked to multiples Thermal Zones. Falling back to using the first Thermal Zone '"
+                                  << zoneOrSpaceName << "'.");
+        }


If we are NOT translating spaces.... We construct a std::set of all the found Thermal Zone Names (so it's sorted and unique)
If we can't find any, throw. Otherwise, grab the first one, issue a warning if one only, an error if two or more

src/energyplus/Test/EMS_GTest.cpp

developer/doc/ReleaseNotes/OpenStudio_Release_Notes_3_6_0_TBD.md

ci-commercialbuildings · 2023-03-15T19:34:39Z

CI Results for 045292d:

Ubuntu 18.04:
Ubuntu 20.04:
Darwin:
- OpenStudio-3.6.0-alpha+045292dd8a-Darwin-x86_64.dmg
- OpenStudio-3.6.0-alpha+045292dd8a-Darwin-x86_64.tar.gz
Windows:
Code Coverage:

brianlball

this is great work! Thank you!

mdahlhausen · 2023-03-15T21:28:50Z

Thanks @jmarrec this is helpful! I'll test it out with standards unless Jeremy Lerond beats me to it.

lymereJ · 2023-03-15T21:30:26Z

Thanks @jmarrec this is helpful! I'll test it out with standards unless Jeremy Lerond beats me to it.

I probably won't, thanks @jmarrec for addressing this quickly!

jmarrec added 8 commits March 15, 2023 17:41

#4786 - EMS: Change the last optional field to Zone or Space Name, st…

d76d8c2

…ore a Space or a ThermalZone in it.

#4786 - EMS: Enforce using the Zone or Space only for SpaceLoadInstan…

0ba67d7

…ce (and vice versa)

#4786 - EMS: Adjust FT, try to be smart about it.

2090039

Adjust existing model tests

f5baa16

new model tests

1a93d49

EMS: When we resolve to a collection of spaces that has multiple obje…

b150431

…cts, sort them by name, so we're deterministic

Adjust FT test

7da2a74

Add a comment in the test where we can end up with a problem.

960833c

jmarrec added component - Model component - IDF Translation Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Mar 15, 2023

jmarrec requested a review from brianlball March 15, 2023 18:34

jmarrec self-assigned this Mar 15, 2023

Release notes

523e9e2

brianlball reviewed Mar 15, 2023

View reviewed changes

src/model/EnergyManagementSystemActuator.hpp Show resolved Hide resolved

jmarrec commented Mar 15, 2023

View reviewed changes

developer/doc/ReleaseNotes/OpenStudio_Release_Notes_3_6_0_TBD.md Show resolved Hide resolved

replace deprecated method, last occurence

045292d

jmarrec force-pushed the 4786_EMS_Actuators_Spaces branch from c5a08ce to 045292d Compare March 15, 2023 19:03

brianlball approved these changes Mar 15, 2023

View reviewed changes

jmarrec merged commit df82458 into develop Mar 16, 2023
2 checks passed

jmarrec deleted the 4786_EMS_Actuators_Spaces branch March 16, 2023 07:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4786 - Space load-based actuator for spaces are named based on thermal zone name #4828

Fix #4786 - Space load-based actuator for spaces are named based on thermal zone name #4828

jmarrec commented Mar 15, 2023 •

edited

jmarrec Mar 15, 2023

jmarrec Mar 15, 2023

brianlball Mar 15, 2023

jmarrec Mar 15, 2023

jmarrec Mar 15, 2023

ci-commercialbuildings commented Mar 15, 2023 •

edited

brianlball left a comment

mdahlhausen commented Mar 15, 2023

lymereJ commented Mar 15, 2023

Fix #4786 - Space load-based actuator for spaces are named based on thermal zone name #4828

Fix #4786 - Space load-based actuator for spaces are named based on thermal zone name #4828

Conversation

jmarrec commented Mar 15, 2023 • edited

Pull request overview

Pull Request Author

Review Checklist

jmarrec Mar 15, 2023

Choose a reason for hiding this comment

jmarrec Mar 15, 2023

Choose a reason for hiding this comment

brianlball Mar 15, 2023

Choose a reason for hiding this comment

jmarrec Mar 15, 2023

Choose a reason for hiding this comment

jmarrec Mar 15, 2023

Choose a reason for hiding this comment

ci-commercialbuildings commented Mar 15, 2023 • edited

brianlball left a comment

Choose a reason for hiding this comment

mdahlhausen commented Mar 15, 2023

lymereJ commented Mar 15, 2023

jmarrec commented Mar 15, 2023 •

edited

ci-commercialbuildings commented Mar 15, 2023 •

edited