Addresses #4403, add Sql helper methods to retrieve U-factors, SHGC, or VT for glazing systems #4550

joseph-robertson · 2022-03-10T17:01:11Z

Pull request overview

Fixes Add Sql helper methods to retrieve U-factors, SHGC, or VT for glazing systems #4403
Companion PR: Addresses #4403, add Sql helper methods to retrieve U-factors, SHGC, or VT for glazing systems OpenStudio-resources#166

Checklist:

for UFactor, SHGC, VisibleTransmittance.

Questions:

Can we somehow combine getFenestrationValue methods from SqlFile and SubSurface so that we don't duplicate code?
Look in TableName=Exterior Fenestration, then TableName=Interior Fenestration? Update: Interior Fenestration wasn't implemented.
How to test new assembly<type>() methods on SubSurface? Update: Use new FrameAndDivider; reverse translate in.idf and attach eplusout.sql to model.

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

jmarrec · 2022-03-14T17:06:36Z

src/model/test/SubSurface_GTest.cpp

@@ -1531,3 +1534,34 @@ TEST_F(ModelFixture, Issue_4361_Multi_Subsurfaces_Overlapping) {
  windowWallRatio = surface.windowToWallRatio();
  EXPECT_NEAR(windowWallRatio, 0.2742, 0.01);
 }
+
+TEST_F(ModelFixture, 4403_FenestrationAssembly) {
+  // Test for #4403 - Add Sql helper methods to retrieve U-factors, SHGC, or VT for glazing systems


I like the test

(Side note: I'm not a big fan on relying on the ReverseTranslator though. Would be better with an OSM directly, but that's probably a non issue)

it makes sense to use RUN_ENERGYPLUS to keep current, so I actually like this solution.

I see pros/cons both ways. The con, as I see it, with OSM directly is that OSM/IDF get out of sync in FrameAndDivider.

jmarrec

At the very least, move the method from Modelobject to SubSurface, but perhaps delete it?

resources/CMakeLists.txt

jmarrec · 2022-03-14T17:09:21Z

src/model/ModelObject.cpp

+    /** Gets the fenestration value from the sql file **/
+    boost::optional<double> ModelObject_Impl::getFenestrationValue(std::string columnName) const {
+      boost::optional<double> result;


Unless there is another class that will need this, this shouldn't be on ModelObject, but just on the SubSurface

I find this method a bit confusing. Do we need this in addition to the better named assemblyUFactor etc?

I'm using getAutosizedValue as a guide, where you can pass in the appropriate columnName.

yeah but in that case it makes sense because lots of child classes get to use that method. Here that doesn't seem to be the case

I don't see the harm - it reduces duplicated code. Should we rename the method to something other than getFenestrationValue?

I changed it to getExteriorFenestrationValue so that it aligns with TableName=Exterior Fenestration.

jmarrec · 2022-03-14T17:10:04Z

src/model/test/SubSurface_GTest.cpp

@@ -1531,3 +1534,34 @@ TEST_F(ModelFixture, Issue_4361_Multi_Subsurfaces_Overlapping) {
  windowWallRatio = surface.windowToWallRatio();
  EXPECT_NEAR(windowWallRatio, 0.2742, 0.01);
 }
+
+TEST_F(ModelFixture, 4403_FenestrationAssembly) {
+  // Test for #4403 - Add Sql helper methods to retrieve U-factors, SHGC, or VT for glazing systems


it makes sense to use RUN_ENERGYPLUS to keep current, so I actually like this solution.

src/utilities/sql/SqlFile.cpp

src/utilities/sql/Test/SqlFile_GTest.cpp

ci-commercialbuildings · 2022-03-16T15:13:47Z

CI Results for b3d13c0:

jmarrec

@joseph-robertson LGTM!

jmarrec · 2022-03-23T16:30:41Z

src/model/SubSurface.hpp

+    /** @name Queries */
+    //@{
+
+    /** Gets the fenestration value from the sql file **/


Maybe a comment here or in the implementation explaining that it only reports for exterior fenestration (and not interior fenestration, and unlike the original NFP) would be helpful.

Oh ok, my bad, the name actually is spot on, getExteriorFenestrationValue. LGTM!

HotFix #4550 - Remove getExteriorFenestrationValue from public API

joseph-robertson added 3 commits March 10, 2022 09:59

Stub sql convenience methods for fenestration assembly.

ae2c298

Stub sql test for fenestration assembly.

fe559bc

Clang format.

a49e815

joseph-robertson added Enhancement Request component - Utilities Other labels Mar 10, 2022

joseph-robertson self-assigned this Mar 10, 2022

joseph-robertson changed the base branch from develop to v22.1.0-IOFreeze March 10, 2022 17:42

joseph-robertson added Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Mar 11, 2022

joseph-robertson added 2 commits March 11, 2022 09:31

Stub methods on subsurface class.

7d98fd9

Add new resources test idf with frame and divider.

23945d6

joseph-robertson mentioned this pull request Mar 11, 2022

Addresses #4403, add Sql helper methods to retrieve U-factors, SHGC, or VT for glazing systems NREL/OpenStudio-resources#166

Merged

24 tasks

joseph-robertson added 2 commits March 11, 2022 14:14

Sql methods are working.

0d06d63

Add new test for subsurface getters.

425ab07

jmarrec reviewed Mar 14, 2022

View reviewed changes

jmarrec requested changes Mar 14, 2022

View reviewed changes

joseph-robertson added 7 commits March 14, 2022 12:23

Merge branch 'v22.1.0-IOFreeze' into issue-4403

ec818b9

Move from modelobject to subsurface, and condense duplicate code.

b887490

Typo.

a522174

Remove query for interior fenestration.

2903f6d

Method name change.

0dcb09c

Missed a name update.

39e45e7

Another name update.

b3d13c0

joseph-robertson requested a review from jmarrec March 16, 2022 16:53

jmarrec approved these changes Mar 23, 2022

View reviewed changes

Base automatically changed from v22.1.0-IOFreeze to develop March 23, 2022 16:40

jmarrec merged commit 6640698 into develop Mar 23, 2022

jmarrec deleted the issue-4403 branch March 23, 2022 16:41

jmarrec mentioned this pull request Mar 23, 2022

HotFix #4550 - Remove getExteriorFenestrationValue from public API #4553

Merged

jmarrec added a commit that referenced this pull request Mar 24, 2022

Merge pull request #4553 from NREL/hotfix/4550

135d278

HotFix #4550 - Remove getExteriorFenestrationValue from public API

jmarrec mentioned this pull request Mar 24, 2022

Adjustments to WindowPropertyFrameAndDivider and new model gtest #4555

Merged

16 tasks

joseph-robertson mentioned this pull request Sep 13, 2022

Glass U Factor sql error #4678

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addresses #4403, add Sql helper methods to retrieve U-factors, SHGC, or VT for glazing systems #4550

Addresses #4403, add Sql helper methods to retrieve U-factors, SHGC, or VT for glazing systems #4550

joseph-robertson commented Mar 10, 2022 •

edited by jmarrec

Loading

jmarrec Mar 14, 2022

jmarrec Mar 14, 2022

joseph-robertson Mar 14, 2022

jmarrec left a comment

jmarrec Mar 14, 2022

jmarrec Mar 14, 2022

joseph-robertson Mar 14, 2022

jmarrec Mar 14, 2022

joseph-robertson Mar 14, 2022

joseph-robertson Mar 15, 2022

jmarrec Mar 14, 2022

ci-commercialbuildings commented Mar 16, 2022 •

edited

Loading

jmarrec left a comment

jmarrec Mar 23, 2022

jmarrec Mar 23, 2022

Addresses #4403, add Sql helper methods to retrieve U-factors, SHGC, or VT for glazing systems #4550

Addresses #4403, add Sql helper methods to retrieve U-factors, SHGC, or VT for glazing systems #4550

Conversation

joseph-robertson commented Mar 10, 2022 • edited by jmarrec Loading

Pull request overview

Pull Request Author

Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Mar 16, 2022 • edited Loading

jmarrec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joseph-robertson commented Mar 10, 2022 •

edited by jmarrec

Loading

ci-commercialbuildings commented Mar 16, 2022 •

edited

Loading