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-08 10:45:47 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.

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-08T08:32:59Z

+    std::vector<MRTWeightingFactor> mrtWeightingFactors() const;
+
+    unsigned int numberofMRTWeightingFactors() const;
+
+    boost::optional<unsigned> mrtWeightingFactorIndex(const People& people) const;
+
+    boost::optional<MRTWeightingFactor> getMRTWeightingFactor(unsigned groupIndex) const;
+
+    bool addMRTWeightingFactor(const MRTWeightingFactor& mrtWeightingFactor);
+
+    bool addMRTWeightingFactor(const People& people, double mrtWeightingFactor);
+
+    bool addMRTWeightingFactors(const std::vector<MRTWeightingFactor>& mrtWeightingFactors);
+
+    void removeMRTWeightingFactor(int groupIndex);
+
+    void removeAllMRTWeightingFactors();


MRT is an accronym.

You use mrtW and MRTW here, which is an unusual way of casing.

I know mRTWeightingFactor looks weirdish, but that's the camelCase convention we're using everywhere else.

I actually rely on it sometimes when doing dynamic programming...

I suggest either:

changing

or motivating it by looking at how we handled other IDD fields with acronyms.

I asked Claude to look into it

In the codebase, when an acronym leads a lowerCamelCase identifier (getter/local var, not prefixed by get/set/add), the established pattern only lowercases the first letter of the acronym, e.g.:

dXCoil, dXHeatingCoilSizingRatio (from IDD field DX...)

eMSVariableName, eMSRuntimeLanguageDebugOutputLevel (from EMS...)

fMUFile, fMUFileName, fMUVariableName (from FMU...)

vFDControlType, vFDEfficiencyCurve (from VFD...)

fCAH, fCAuxHeat, fCStorage (from FC..., Generator:FuelCell fields)

That's 49 distinct identifiers following this rule, vs. only 3 outliers (copFunctionofTemperatureCurve and friends, which fully lowercase COP) — likely older/manually-typed exceptions rather than the intended convention. So the dominant, established rule is: lowercase only the leading letter of the acronym, keep the rest capitalized — exactly the mRTWeightingFactor you described.

ZoneMRTCalculation currently uses mrtWeightingFactor/mrtWeightingFactors/mrtWeightingFactorIndex (member functions, local vars, and the private member m_mrtWeightingFactor), which breaks that convention. The class name MRTWeightingFactor itself and the prefixed methods (addMRTWeightingFactor, getMRTWeightingFactor, numberofMRTWeightingFactors, removeMRTWeightingFactor) are already correctly cased and don't need touching — only the un-prefixed lowerCamelCase spots do.

Done in 18cb9ea

jmarrec · 2026-09-08T09:12:57Z

+  /** This class implements an MRT weighting factor. */
+  class MODEL_API MRTWeightingFactor
+  {
+   public:
+    MRTWeightingFactor(const People& people, double mrtWeightingFactor);
+
+    People people() const;
+    double mrtWeightingFactor() const;
+
+   private:
+    People m_people;
+    double m_mrtWeightingFactor;
+    REGISTER_LOGGER("openstudio.model.MRTWeightingFactor");
+  };


Should we add an equality operator?

If so, do we compare People only? (a people can only be listed once...)
Or on both People and factor? (which is semantically more correct/expected)

🤔

Added an operator== that compares on People AND factor. be24456

jmarrec · 2026-09-08T09:21:18Z

+      if (sum > 1.0) {
+        LOG(Error, "Cannot add " << people.briefDescription() << " to " << briefDescription() << " because the MRT Weighting Factors would sum to "
+                                 << sum << ", which is greater than 1.");
+        return result;
+      }


AH! While modifying the OpenStudio-resources tests to be more comprehensive, I got bit by this floating point comparison. We need a tolerance.

https://github.com/NatLabRockies/EnergyPlus/blob/5b898844e92b3718ab91b858c77eaf8782eb56b6/src/EnergyPlus/HeatBalanceManager.cc#L2468-L2473

jmarrec · 2026-09-08T09:50:16Z

 MODELOBJECT_TEMPLATES(SurfacePropertyExposedFoundationPerimeter);
 MODELOBJECT_TEMPLATES(ViewFactor); // Helper class defined in ZonePropertyUserViewFactorsBySurfaceName
 MODELOBJECT_TEMPLATES(ZonePropertyUserViewFactorsBySurfaceName);
+MODELOBJECT_TEMPLATES(MRTWeightingFactor); // Helper class defined in ZoneMRTCalculation


You have defined operator<< for that class, you need to extend str so Ruby/Python see it

…m, per review.

…tWeightingFactorIndex to use just People, update model tests.

…s wrong zone.

cf #5649 (comment)

…ng on both People and Factor (

…Zone::getZoneMRTCalculation

…expansive machinery like this for such a niche object.

jmarrec · 2026-09-08T13:40:48Z

@joseph-robertson I have updated the PR as well as the OpenStudio-resources test at NatLabRockies/OpenStudio-resources#232

Unfortunately the Ubuntu runners have a Github CLI EXPKEYSIG (I got it locally on Ubuntu 24.04 too), so we can have CI run the OS resources test with the deb installer from this PR, but I built locally and confirmed everything works fine.

Can you take a quick pass at my changes? Then we can drop both. Thank you.

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 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

Comment thread src/energyplus/ForwardTranslator/ForwardTranslateZoneMRTCalculation.cpp Outdated

Comment thread src/model/ZoneMRTCalculation.cpp Outdated

Comment thread src/model/ZoneMRTCalculation.cpp Outdated

Comment thread src/model/ZoneMRTCalculation.cpp Outdated

joseph-robertson commented Sep 2, 2026

View reviewed changes

Comment thread src/model/People.cpp Outdated

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 requested a review from jmarrec September 2, 2026 16:43

jmarrec requested changes Sep 4, 2026

View reviewed changes

joseph-robertson mentioned this pull request Sep 4, 2026

Wrap ZoneMRTCalculation NatLabRockies/OpenStudio-resources#232

Draft

42 tasks

jmarrec requested changes Sep 8, 2026

View reviewed changes

jmarrec reviewed Sep 8, 2026

View reviewed changes

jmarrec requested changes Sep 8, 2026

View reviewed changes

joseph-robertson added 11 commits September 8, 2026 11:57

Update both IDDs for new ZoneMRTCalculation object.

6b1dfad

Add model files for new ZoneMRTCalculation object.

d514593

Add ft and rt for new ZoneMRTCalculation object.

6cc268d

Add model and translation tests for new ZoneMRTCalculation object.

c96fd71

Expand new tests to cover more throw/error scenarios.

d0a5473

Another pass through model and tests with additions and cleanups.

5b1cefb

Address updates in ft and test, and model cpp.

d1f1599

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

c6cf665

…m, per review.

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

c9d1344

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

4b33de1

…tWeightingFactorIndex to use just People, update model tests.

Remove alpha type from Thermal Zone Name field in idd.

fe4453b

joseph-robertson and others added 14 commits September 8, 2026 11:57

Clean typo in clone log and throw.

1cf61e4

Update ThermalZone with ZoneMRT remove and new output variable.

a28c55f

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

1106a45

Simplify ostream by removing report of People iddObject.

4cd635f

Do the ZoneMRTCalc ft from thermal zone.

b8a1ca3

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

a8f2f03

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

9e1a860

…s wrong zone.

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

5b5df9d

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

cf0f8a5

Use structured bindings in extensible groups ft.

6faf3fa

Use camelCase for mRTWeightingFactor (not mrtWeightingFactor)

18cb9ea

cf #5649 (comment)

Need a tolerance to compare sum > 1.0

c84b050

Add operator== / operator!= for MRTWeightFactor helper class, compari…

be24456

…ng on both People and Factor (

SWIG extend __str__ for operator<< usage in helper class

ed3042d

jmarrec force-pushed the v26.2.0-IOFreeze-ZoneMRTCalc branch from 723ae70 to ed3042d Compare September 8, 2026 10:19

jmarrec added 2 commits September 8, 2026 12:44

Make ZoneMRTCalculation Ctor protected to force using through Thermal…

d4fb3f4

…Zone::getZoneMRTCalculation

Remove commented out hooks in People for ZoneMRTCalc:no point having …

4e1f119

…expansive machinery like this for such a niche object.

jmarrec self-requested a review September 8, 2026 13:45

jmarrec approved these changes Sep 8, 2026

View reviewed changes

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarrec Sep 8, 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!

jmarrec Sep 8, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec commented Sep 8, 2026

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

jmarrec Sep 8, 2026 •

edited

Loading

jmarrec Sep 8, 2026 •

edited

Loading