Skip to content

V26.2.0-IOFreeze: new Absorptance Inside Face fields - #5648

Merged
jmarrec merged 9 commits into
developfrom
v26.2.0-IOFreeze-InsideFaceAbs
Sep 4, 2026
Merged

V26.2.0-IOFreeze: new Absorptance Inside Face fields#5648
jmarrec merged 9 commits into
developfrom
v26.2.0-IOFreeze-InsideFaceAbs

Conversation

@joseph-robertson

@joseph-robertson joseph-robertson commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Pull request overview

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

@joseph-robertson joseph-robertson self-assigned this Sep 1, 2026
@joseph-robertson 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
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🧪 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 10:05:44 UTC

❌ Significant Test Failures

📊 Test Run Information

Run XML File Status
- No XML files found ❌ Missing

@joseph-robertson
joseph-robertson requested a balanced review from Copilot September 1, 2026 21:47
@joseph-robertson
joseph-robertson marked this pull request as ready for review September 1, 2026 21:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The blank Material:NoMass reverse-translation test uses incorrect field indices, invalidating its fallback assertions.

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

Pull request overview

Adds inside-face absorptance support for standard and massless opaque materials, including blank-value inheritance and EnergyPlus translation.

Changes:

  • Extends the model schema and APIs with three inside-face absorptance fields.
  • Adds forward and reverse translation support.
  • Adds model and translator tests.
File summaries
File Description
resources/model/OpenStudio.idd Defines the optional fields.
src/model/StandardOpaqueMaterial.hpp Exposes the standard-material API.
src/model/StandardOpaqueMaterial_Impl.hpp Declares implementation methods.
src/model/StandardOpaqueMaterial.cpp Implements fallback and mutation behavior.
src/model/MasslessOpaqueMaterial.hpp Exposes the massless-material API.
src/model/MasslessOpaqueMaterial_Impl.hpp Declares implementation methods.
src/model/MasslessOpaqueMaterial.cpp Implements fallback and mutation behavior.
src/model/test/StandardOpaqueMaterial_GTest.cpp Tests standard-material behavior.
src/model/test/MasslessOpaqueMaterial_GTest.cpp Tests massless-material behavior.
src/energyplus/ForwardTranslator/ForwardTranslateStandardOpaqueMaterial.cpp Writes explicit standard-material values.
src/energyplus/ForwardTranslator/ForwardTranslateMasslessOpaqueMaterial.cpp Writes explicit massless-material values.
src/energyplus/ReverseTranslator/ReverseTranslateMaterial.cpp Imports standard-material values.
src/energyplus/ReverseTranslator/ReverseTranslateMaterialNoMass.cpp Imports massless-material values.
src/energyplus/Test/ForwardTranslator_GTest.cpp Tests omission and emission behavior.
src/energyplus/Test/ReverseTranslator_GTest.cpp Tests imports and blank-field fallback.
Review details
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Balanced

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

Comment thread src/energyplus/Test/ReverseTranslator_GTest.cpp Outdated
@joseph-robertson

joseph-robertson commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

NatLabRockies/EnergyPlus#11750 adds new output variables:

  • Surface Thermal Absorptance
  • Surface Solar Absorptance
  • Surface Thermal Absorptance Outside Face
  • Surface Solar Absorptance Outside Face
  • Surface Thermal Absorptance Inside Face
  • Surface Solar Absorptance Inside Face

But I'm not sure whether these should be added to any outputVariableNames; Surface.cpp contains only "Surface Inside Face Temperature" and "Surface Outside Face Temperature" while there are several more existing output variables that could be listed?

@jmarrec

jmarrec commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

But I'm not sure whether these should be added to any outputVariableNames; Surface.cpp contains only "Surface Inside Face Temperature" and "Surface Outside Face Temperature" while there are several more existing output variables that could be listed?

Please do. I blamed, the Surface_Impl::outputVariableNames hasn't been touched since the first git commit 13 years ago. Just add all output variables for Surface in there

Edit: done in b0a5595 Add full outputVariableNames for Surface/SubSurface

@jmarrec jmarrec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines +2241 to +2258
N8, \field Thermal Absorptance Inside Face
\note When blank, this value is equal to the Thermal Absorptance field above.
\note When entered, the Thermal Absorptance field applies to the outside face and this field applies to the inside face.
\type real
\minimum> 0
\maximum 0.99999
N9, \field Solar Absorptance Inside Face
\note When blank, this value is equal to the Solar Absorptance field above.
\note When entered, the Solar Absorptance field applies to the outside face and this field applies to the inside face.
\type real
\minimum 0
\maximum 1
N10; \field Visible Absorptance Inside Face
\note When blank, this value is equal to the Visible Absorptance field above.
\note When entered, the Visible Absorptance field applies to the outside face and this field applies to the inside face.
\type real
\minimum 0
\maximum 1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, truly optional. and No VT needed

Comment on lines +102 to +104
double thermalAbsorptanceInsideFace() const;

bool isThermalAbsorptanceInsideFaceDefaulted() const;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I'm guessing you went with if blank, grab the (regular) thermalAbsorptance field?

Comment on lines +194 to +203
double StandardOpaqueMaterial_Impl::thermalAbsorptanceInsideFace() const {
if (boost::optional<double> value = getDouble(OS_MaterialFields::ThermalAbsorptanceInsideFace)) {
return value.get();
}
return thermalAbsorptance();
}

bool StandardOpaqueMaterial_Impl::isThermalAbsorptanceInsideFaceDefaulted() const {
return isEmpty(OS_MaterialFields::ThermalAbsorptanceInsideFace);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, yeah. I guess that's indeed a valid choice of implementation.

The other one being just provide an boost::optional<double> getter, setter and resetter.

Comment on lines +61 to +64
// Inside-face absorptance fields have no IDD default and inherit the exterior value when blank.
if (!modelObject.isThermalAbsorptanceInsideFaceDefaulted()) {
idfObject.setDouble(openstudio::MaterialFields::ThermalAbsorptanceInsideFace, modelObject.thermalAbsorptanceInsideFace());
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, only write it if it's explicitly set, makes sense

Comment thread src/model/Surface.cpp
Comment on lines 164 to +173
const std::vector<std::string>& Surface_Impl::outputVariableNames() const {
static const std::vector<std::string> result{"Surface Inside Face Temperature", "Surface Outside Face Temperature"};
static const std::vector<std::string> result{
// Face temperatures and convection
"Surface Inside Face Temperature",
"Surface Inside Face Interior Movable Insulation Temperature",
"Surface Outside Face Temperature",
"Surface Inside Face Adjacent Air Temperature",
"Surface Inside Face Convection Heat Transfer Coefficient",
"Surface Inside Face Convection Heat Gain Rate",
"Surface Inside Face Convection Heat Gain Rate per Area",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added 98 output Variables for Surface, 169 for SubSurfaces. From the E+ source code itself.

Note: Noticed the I/O ref is out of date, filled NatLabRockies/EnergyPlus#11783 (comment)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the following included in your add:

  • Surface Thermal Absorptance Outside Face
  • Surface Solar Absorptance Outside Face
  • Surface Thermal Absorptance Inside Face
  • Surface Solar Absorptance Inside Face

Is that intentional?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that from the c++ source code or the I/O reference?

(The I/O reference is quite outdated as I said)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's new stuff in the E+ v26.2 IOFreeze.

@jmarrec

jmarrec commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Just going to let CI take a run on it to make sure my Surface/SubSurface edits are ok.

@joseph-robertson if you come around to this PR before me and Full Build is happy, go ahead and merge!

@jmarrec
jmarrec merged commit 29ea277 into develop Sep 4, 2026
6 of 7 checks passed
@jmarrec
jmarrec deleted the v26.2.0-IOFreeze-InsideFaceAbs branch September 4, 2026 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

IDDChange Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants