V26.2.0-IOFreeze: new ZoneMRTCalculation object by joseph-robertson · Pull Request #5649 · NatLabRockies/OpenStudio

joseph-robertson · 2026-09-01T21:36:29Z

Pull request overview

Wraps Extension of MRT Calculation Options Beyond Thermal Comfort Models EnergyPlus#11718

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: Wrap ZoneMRTCalculation OpenStudio-resources#232
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

github-actions · 2026-09-01T21:36:56Z

🧪 Test Results Dashboard

Summary

Metric	Value
Total Tests	0
Passed	0
Failed	0
Errors	0
Skipped	0
Success Rate	0.0%
Generated	2026-09-04 18:12:07 UTC

❌ Significant Test Failures

📊 Test Run Information

Run	XML File	Status
-	No XML files found	❌ Missing

Copilot

🟡 Changes recommended

Weight-sum validation, stale references, non-finite values, and partial-add failure reporting can currently produce invalid behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds Model API and EnergyPlus translation support for ZoneMRTCalculation.

Changes:

Adds the model object, MRT weighting-factor API, and ThermalZone integration.
Adds forward/reverse translators and binding support.
Updates IDDs, build registration, and tests.

File summaries

File	Description
`src/model/ZoneMRTCalculation.hpp`	Declares the new API.
`src/model/ZoneMRTCalculation.cpp`	Implements weighting-factor behavior.
`src/model/ZoneMRTCalculation_Impl.hpp`	Declares implementation details.
`src/model/ThermalZone.hpp`	Exposes MRT calculation access.
`src/model/ThermalZone.cpp`	Implements creation and lookup.
`src/model/ThermalZone_Impl.hpp`	Adds implementation declaration.
`src/model/test/ZoneMRTCalculation_GTest.cpp`	Tests the model API.
`src/model/ModelHVAC.i`	Adds C# ThermalZone access.
`src/model/ModelGeometry.i`	Registers binding templates.
`src/model/Model.cpp`	Registers constructors.
`src/model/ConcreteModelObjects.hpp`	Includes the new object.
`src/model/CMakeLists.txt`	Adds model sources and tests.
`src/energyplus/Test/ZoneMRTCalculation_GTest.cpp`	Tests both translators.
`src/energyplus/ReverseTranslator/ReverseTranslateZoneMRTCalculation.cpp`	Implements reverse translation.
`src/energyplus/ReverseTranslator.hpp`	Declares reverse translation.
`src/energyplus/ReverseTranslator.cpp`	Dispatches reverse translation.
`src/energyplus/ForwardTranslator/ForwardTranslateZoneMRTCalculation.cpp`	Implements forward translation.
`src/energyplus/ForwardTranslator.hpp`	Declares forward translation.
`src/energyplus/ForwardTranslator.cpp`	Registers forward translation.
`src/energyplus/CMakeLists.txt`	Adds translator sources and tests.
`resources/model/OpenStudio.idd`	Defines the OpenStudio object.
`resources/energyplus/ProposedEnergy+.idd`	Converts EnergyPlus fields to extensible groups.

Review details

Suppressed comments (1)

src/model/ZoneMRTCalculation.cpp:107

Removing a referenced People object clears this pointer but leaves the extensible group. A subsequent call to addMRTWeightingFactor reaches this predicate and calls .get() on an empty optional, throwing instead of adding the new factor. Treat a missing field as a non-match (and add a removal regression test).

      auto it = std::find_if(egs.begin(), egs.end(), [&](const WorkspaceExtensibleGroup& eg) {
        return eg.getField(OS_ZoneMRTCalculationExtensibleFields::PeopleName).get() == peopleHandle;
      });

Files reviewed: 22/22 changed files
Comments generated: 4
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

joseph-robertson · 2026-09-02T04:16:12Z

+    for (const MRTWeightingFactor& mrtWeightingFactor : mrtWeightingFactors) {
+      People people = mrtWeightingFactor.people();
+      boost::optional<IdfObject> _people = translateAndMapModelObject(people);
+
+      if (_people) {


joseph-robertson · 2026-09-02T04:16:39Z

+        return result;
+      }
+
+      boost::optional<unsigned> existingIndex = mrtWeightingFactorIndex(mrtWeightingFactor);


…m, per review.

…tWeightingFactorIndex to use just People, update model tests.

joseph-robertson · 2026-09-02T04:19:59Z

+    // The hooks below would remove ZoneMRTCalculation extensible groups as soon as a referenced People object
+    // becomes invalid by being moved to another Space, reset from its Space, assigned to a SpaceType, or losing
+    // its thermal comfort model types. That is more proactive than most existing extensible-reference patterns in
+    // the model, which generally clean references only from the referenced object's remove() path and otherwise
+    // rely on explicit remove APIs or read/translation-time filtering.


Not sure if we'd want this -- it may be overkill.

Copilot

🟡 Changes recommended

Thermal-zone deletion can leave an invalid ZoneMRTCalculation object that later causes assertions during access or translation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

resources/model/OpenStudio.idd:6830

This field declares two conflicting \type values; all other OpenStudio object-list fields use only \type object-list (for example, lines 6808-6809 and 6837-6838). Remove the redundant alpha declaration so the IDD metadata has one unambiguous field type.
src/model/ZoneMRTCalculation.cpp:85
These adjacent string literals concatenate as andthat, making the thrown clone error malformed. Preserve a space at the literal boundary.

Files reviewed: 25/25 changed files
Comments generated: 1
Review effort level: Balanced

joseph-robertson · 2026-09-02T05:15:59Z

+    ZoneMRTCalculation ThermalZone_Impl::getZoneMRTCalculation() const {
+      auto thisThermalZone = getObject<ThermalZone>();
+      std::vector<ZoneMRTCalculation> zoneMRTCalculations =
+        thisThermalZone.getModelObjectSources<ZoneMRTCalculation>(ZoneMRTCalculation::iddObjectType());


I think we'd have the same issue when deleting a Space.

Removing a Space bulk-removes its child People through ParentObject_Impl::remove(), but it does not call People_Impl::remove(). So cleanup would require Space_Impl::remove() to inspect all child People, then find and prune any ZoneMRTCalculation groups for each one. That reaches across more ownership boundaries and is less consistent with the existing bulk child-removal mechanism.

So: zone removal is direct source-object cleanup; space removal is indirect child/reference cleanup.

This handles ThermalZone: 435380b

jmarrec

Looking pretty good. A couple of things to change in the code review

And please fill out the PR checklist, most important item of it: we need a OpenStudio-resources matching test for this new object

jmarrec · 2026-09-04T10:23:45Z

+       \note A People object assigned directly to a Space retains its input name.
+       \note A People object expanded across multiple Spaces must use an expanded instance name
+       \note formed as "<Space Name> <People Name>"; its original input name is not accepted.
+       \note People instances in automatically generated remainder Spaces are not supported.


That scares me

Yeah, not totally sure what to do here. If the model has People object that references a SpaceType (instead of a Space), then the IDF's People object references a SpaceList. So I think we'd want to support this (somehow), and the resulting IDF's ZoneMRTCalculation object would look something like (?)

ZoneMRTCalculation, [...] Story 1 East Perimeter Space Baseline Model People, !- People 1 Name 0.25, !- MRT Weighting Factor 1 Story 1 West Perimeter Space Baseline Model People, !- People 2 Name 0.75, !- MRT Weighting Factor 2

I think your current implementation which enforces things at model time plus FT time is fine
More importantly this is going to be a very niche object, so if you choose to use it you can also deal with the preconditions.
For eg I don't think we need to check is assigned to a spaceType and the sp has only one space

jmarrec · 2026-09-04T10:32:14Z

+     * This constructor shouldn't be used, ThermalZone::getZoneMRTCalculation should be preferred
+     * as it will either instantiate a new object or return the existing one.
+     * A ThermalZone should not have more than one ZoneMRTCalculation. If you try to instantiate a second one for a given
+     * ThermalZone this constructor will throw.
+     */
+    explicit ZoneMRTCalculation(const ThermalZone& thermalZone);


Ok, I follow.

But since you rightfully want the constructor to NOT be used, make it protected, and friend ThermalZone_Impl

Should we do the same for ZonePropertyUserViewFactorsBySurfaceName?

jmarrec · 2026-09-04T10:32:50Z

+
+    ThermalZone thermalZone() const;
+
+    std::vector<MRTWeightingFactor> mrtWeightingFactors() const;


This is a nitpick, but I like to handle all extensible fields next to each other inside the @other section

jmarrec · 2026-09-04T10:35:56Z

+  // Direct construction is intentionally blocked once the ThermalZone already has one object.
+  EXPECT_THROW((ZoneMRTCalculation(thermalZone)), openstudio::Exception);
+  EXPECT_EQ(size + 1, model.modelObjects().size());


This shouldn't compile after you make it protected

jmarrec · 2026-09-04T10:59:03Z

+      boost::optional<ThermalZone> thermalZone = space->thermalZone();
+      if (!thermalZone || (thermalZone->handle() != this->thermalZone().handle())) {
+        LOG(Error, "Cannot add " << people.briefDescription() << " to " << briefDescription() << " because it is not assigned to the ThermalZone.");
+        return result;
+      }


Suggested change

boost::optional<ThermalZone> thermalZone = space->thermalZone();

if (!thermalZone || (thermalZone->handle() != this->thermalZone().handle())) {

LOG(Error, "Cannot add " << people.briefDescription() << " to " << briefDescription() << " because it is not assigned to the ThermalZone.");

return result;

}

boost::optional<ThermalZone> thermalZone = space->thermalZone();

if (!thermalZone) {

LOG(Error, "Cannot add " << people.briefDescription() << " to " << briefDescription() << " because it is not assigned to any ThermalZone.");

} else if (thermalZone->handle() != this->thermalZone().handle()) {

LOG(Error, "Cannot add " << people.briefDescription() << " to " << briefDescription() << " because it is assigned to ThermalZone '" << thermalZone->nameString() << "' instead of '" << this->thermalZone().nameString() << "'.");

return result;

}

jmarrec · 2026-09-04T11:00:07Z

+    for (const MRTWeightingFactor& mrtWeightingFactor : mrtWeightingFactors) {
+      People people = mrtWeightingFactor.people();
+
+      boost::optional<Space> space = people.space();
+      boost::optional<ThermalZone> thermalZone;
+      if (space) {
+        thermalZone = space->thermalZone();
+      }


You need to throw and skip if !people.space() to begin with, saying it must be part of a space.

I know you're incidentally doing this (if people.space() is empty, so will thermalZone), but it's not as clear as it can be

jmarrec · 2026-09-04T11:03:25Z

+          continue;
+        }
+
+        translatedMRTWeightingFactors.emplace_back(_people->name().get(), value);


Nitpick: use nameString() everywhere

jmarrec · 2026-09-04T11:04:30Z

+    for (const auto& translatedMRTWeightingFactor : translatedMRTWeightingFactors) {
+      auto eg = idfObject.pushExtensibleGroup();
+      eg.setString(ZoneMRTCalculationExtensibleFields::PeopleName, translatedMRTWeightingFactor.first);
+      eg.setDouble(ZoneMRTCalculationExtensibleFields::MRTWeightingFactor, translatedMRTWeightingFactor.second);
+    }


Nitpick: structured bindings for the win

Suggested change

for (const auto& translatedMRTWeightingFactor : translatedMRTWeightingFactors) {

auto eg = idfObject.pushExtensibleGroup();

eg.setString(ZoneMRTCalculationExtensibleFields::PeopleName, translatedMRTWeightingFactor.first);

eg.setDouble(ZoneMRTCalculationExtensibleFields::MRTWeightingFactor, translatedMRTWeightingFactor.second);

}

for (const auto& [name, weight] : translatedMRTWeightingFactors) {

auto eg = idfObject.pushExtensibleGroup();

eg.setString(ZoneMRTCalculationExtensibleFields::PeopleName, name);

eg.setDouble(ZoneMRTCalculationExtensibleFields::MRTWeightingFactor, weight);

}

…s wrong zone.

joseph-robertson added 5 commits September 1, 2026 14:07

Update both IDDs for new ZoneMRTCalculation object.

afefd66

Add model files for new ZoneMRTCalculation object.

03627f1

Add ft and rt for new ZoneMRTCalculation object.

23d26d2

Add model and translation tests for new ZoneMRTCalculation object.

0c8684f

Expand new tests to cover more throw/error scenarios.

5e92cbc

joseph-robertson self-assigned this Sep 1, 2026

joseph-robertson added Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. IDDChange labels Sep 1, 2026

joseph-robertson added 2 commits September 1, 2026 14:44

Another pass through model and tests with additions and cleanups.

222ac39

Address updates in ft and test, and model cpp.

0f9a938

joseph-robertson requested a balanced review from Copilot September 1, 2026 22:20

joseph-robertson marked this pull request as ready for review September 1, 2026 22:20

Copilot started reviewing on behalf of joseph-robertson September 1, 2026 22:20 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

joseph-robertson added 3 commits September 1, 2026 15:52

Updates related to stale people references, nan, weighting factors su…

d56eff7

…m, per review.

Override people remove so we can modify zone mrt extensible groups.

c5bc900

Add optional proactive extensible group removal for People, update mr…

445c0f6

…tWeightingFactorIndex to use just People, update model tests.

joseph-robertson commented Sep 2, 2026

View reviewed changes

joseph-robertson requested a balanced review from Copilot September 2, 2026 04:32

Copilot started reviewing on behalf of joseph-robertson September 2, 2026 04:32 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

joseph-robertson added 3 commits September 2, 2026 09:04

Remove alpha type from Thermal Zone Name field in idd.

5ac4ad6

Clean typo in clone log and throw.

4b08232

Update ThermalZone with ZoneMRT remove and new output variable.

435380b

joseph-robertson requested a review from jmarrec September 2, 2026 16:43

jmarrec requested changes Sep 4, 2026

View reviewed changes

joseph-robertson added 3 commits September 4, 2026 07:58

Merge branch 'develop' into v26.2.0-IOFreeze-ZoneMRTCalc

cb264fb

Move all extensible fields next to each other inside the Other section.

a64f4bf

Simplify ostream by removing report of People iddObject.

b48bfcf

joseph-robertson added 6 commits September 4, 2026 10:55

Do the ZoneMRTCalc ft from thermal zone.

57f9afb

Throw a warn instead of error for ft when no mrt weighting factors.

d001e27

Throw separate errors when adding mrt weighting factors for no zone v…

67e1535

…s wrong zone.

Throw and skip in ft if people not assigned to a space.

85e3783

Use people nameString() instead of name().get() in ft.

51fc2fb

Use structured bindings in extensible groups ft.

723ae70

joseph-robertson mentioned this pull request Sep 4, 2026

Wrap ZoneMRTCalculation NatLabRockies/OpenStudio-resources#232

Draft

42 tasks


		ThermalZone thermalZone() const;

		std::vector<MRTWeightingFactor> mrtWeightingFactors() const;

Uh oh!

Conversation

joseph-robertson commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Pull Request Author

Review Checklist

Uh oh!

github-actions Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧪 Test Results Dashboard

Summary

❌ Significant Test Failures

📊 Test Run Information

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joseph-robertson Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

joseph-robertson commented Sep 1, 2026 •

edited

Loading

github-actions Bot commented Sep 1, 2026 •

edited

Loading

joseph-robertson Sep 2, 2026 •

edited

Loading

jmarrec Sep 4, 2026 •

edited

Loading