Addresses #4380, completing SDK support of Tubular Daylighting Devices (TDDs) #4382

joseph-robertson · 2021-07-23T19:47:28Z

Pull request overview

DO NOT MERGE YET
Fixes Completing SDK support of Tubular Daylighting Devices (TDDs) #4380
Companion regression test: Addresses #4380, completing SDK support of Tubular Daylighting Devices (TDDs) OpenStudio-resources#149
Wraps:
- DaylightingDevice:Tubular
- DaylightingDevice:LightWell
Also update:
- SubSurface
TODO:
- LightWell ctor
- Model tests
- FT tests
Questions:
- Should we have convenience methods dome.addDaylightingDeviceTubular() and diffuser.addDaylightingDeviceTubular() which create default diffuser and dome objects, respectively?
- Should we have setters for sub surfaces?

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: Addresses #4380, completing SDK support of Tubular Daylighting Devices (TDDs) OpenStudio-resources#149
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

…e it makes more sense there anyways + implement C# partial classes for DaylightingDeviceTubular methods that deal with ThermalZone and TransitionZone

…l<std::string>

src/model/DaylightingDeviceTubular.hpp

src/energyplus/ForwardTranslator/ForwardTranslateDaylightingDeviceLightWell.cpp

src/energyplus/ForwardTranslator/ForwardTranslateDaylightingDeviceTubular.cpp

src/model/ModelGeometry.i

src/model/ModelHVAC.i

src/model/ShadowCalculation.cpp

src/model/SubSurface.cpp

src/model/ThermalZone.cpp

…SubSurfaceType, cleanup imports

jmarrec · 2021-08-17T20:57:46Z

src/model/DaylightingDeviceTubular.hpp

+    explicit DaylightingDeviceTubular(const SubSurface& dome, const SubSurface& diffuser, const ConstructionBase& construction, double diameter,
+                                      double totalLength, double effectiveThermalResistance);


This Ctor isn't needed. The one below is enough. you can call the setters for the numeric fields. If you had zero idea about providing a default for these (that's not the case since you made one where you default them), then it would have been required to make this one.
The one is tricky to use too because it's easy to mix up the order of the double arguments.
Also, that means more code to maintain and test.

That being said, if you feel strongly you find it useful, you can leave it.

jmarrec · 2021-08-18T07:55:05Z

src/model/SubSurface.cpp

-          boost::optional<DaylightingDeviceShelf> shelf = this->daylightingDeviceShelf();
-          if (shelf) {
+          if (boost::optional<DaylightingDeviceShelf> shelf = this->daylightingDeviceShelf()) {
+            LOG(Warn, briefDescription() << " new subSurfaceType='" << subSurfaceType << "' is incompatible with Daylighting Device Shelf. Removing the Shelf object.");


FYI I changed the messages in this file @joseph-robertson

jmarrec · 2021-08-18T07:57:09Z

src/model/test/DaylightingDeviceLightWell_GTest.cpp

+  EXPECT_EQ("Door", door.subSurfaceType());
+  EXPECT_EQ(0, model.getConcreteModelObjects<DaylightingDeviceLightWell>().size());
+
+  EXPECT_ANY_THROW({DaylightingDeviceLightWell lightWell(door);});


@joseph-robertson note that gtest has macros for that. EXPECT/ASSERT_ANY/NO_THROW are the most common. You could also do EXPECT_THROW(..., openstudio::Exception) if you want a specific exception

jmarrec · 2021-08-18T07:59:24Z

I made minor changes to what is otherwise a nice clean PR. Will tel CI churn, then make sure the NREL/OpenStudio-resources#149 is still passing. And that this can merge

ci-commercialbuildings · 2021-08-18T09:35:58Z

CI Results for 503e955:

Ubuntu 18.04:
Ubuntu 20.04:
Darwin:
Windows:
- OpenStudio-3.2.2-alpha+503e955da0-Windows.exe
- OpenStudio-3.2.2-alpha+503e955da0-Windows.tar.gz

jmarrec · 2021-08-18T11:02:03Z

I'm happy about this PR. The Reg test should be adjusted but that can happen in a second pass.

joseph-robertson added 4 commits July 23, 2021 12:20

Add tubular and lightwell to idd.

ecfcec8

Stub model files.

9012e96

Update new model files.

c610d36

Stub new methods in subsurface.

3569499

joseph-robertson added Enhancement Request component - Model labels Jul 23, 2021

joseph-robertson self-assigned this Jul 23, 2021

joseph-robertson added IDDChange Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. and removed IDDChange labels Jul 23, 2021

joseph-robertson added 10 commits July 23, 2021 15:37

Stub model tests.

f388bdd

Stub ft files.

99c5422

Stub ft tests.

08025a5

Update cmakelists, etc.

0be5a99

Apply clang.

bcfc6cb

Fill out subsurface methods.

97c4b4b

Refine subsurface methods.

b060d60

Fill out tubular methods.

2040ef0

Fill out lightwell methods.

b7a3b85

Fill out ft.

495828c

joseph-robertson mentioned this pull request Aug 2, 2021

Addresses #4380, completing SDK support of Tubular Daylighting Devices (TDDs) NREL/OpenStudio-resources#149

Merged

42 tasks

joseph-robertson added 9 commits August 2, 2021 11:57

Merge branch 'develop' into issue-4380

3b9823d

Update ctor for light well.

2845b71

Fill out model tests.

aa3659e

Add construction to tubular model test.

2b18dca

Add convenience ctors.

902f8c0

Fill out ft tests.

2321337

Clean up model and ft tests.

1efb7de

Update subsurface set type method.

78ea8a4

Update tubular model files.

922c00b

joseph-robertson and others added 7 commits August 3, 2021 09:42

Update model tests.

603b5a9

Update ft tests.

39dd3c3

Add transition zone to model geometry properly.

2d1fd79

Merge remote-tracking branch 'origin/develop' into issue-4380

050e195

C# SWIG: Move TransitionZone helper class to ThermalZone.hpp/cpp sinc…

a1cdd8c

…e it makes more sense there anyways + implement C# partial classes for DaylightingDeviceTubular methods that deal with ThermalZone and TransitionZone

Fix an issue unrelated to this PR: trying to output an boost::optiona…

322902f

…l<std::string>

clang format

1de173a

jmarrec reviewed Aug 9, 2021

View reviewed changes

src/model/DaylightingDeviceTubular.hpp Show resolved Hide resolved

jmarrec reviewed Aug 9, 2021

View reviewed changes

joseph-robertson added 2 commits August 10, 2021 10:42

Translate and map sub surfaces in ft.

071bd80

Warn when changing subsurface type and removing daylighting device.

bd96477

joseph-robertson requested a review from jmarrec August 13, 2021 19:49

joseph-robertson and others added 2 commits August 13, 2021 12:51

Merge branch 'develop' into issue-4380

87787d3

Minor changes during code review. Enhance messages in SubSurface::set…

8f7103c

…SubSurfaceType, cleanup imports

jmarrec force-pushed the issue-4380 branch from 503e955 to 8f7103c Compare August 18, 2021 07:39

jmarrec approved these changes Aug 18, 2021

View reviewed changes

clang format [chore]

8c1835f

jmarrec merged commit 5193dd1 into develop Aug 18, 2021

jmarrec deleted the issue-4380 branch August 18, 2021 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addresses #4380, completing SDK support of Tubular Daylighting Devices (TDDs) #4382

Addresses #4380, completing SDK support of Tubular Daylighting Devices (TDDs) #4382

joseph-robertson commented Jul 23, 2021 •

edited by jmarrec

Loading

jmarrec Aug 17, 2021

jmarrec Aug 18, 2021

jmarrec Aug 18, 2021

jmarrec commented Aug 18, 2021

ci-commercialbuildings commented Aug 18, 2021 •

edited

Loading

jmarrec commented Aug 18, 2021

		explicit DaylightingDeviceTubular(const SubSurface& dome, const SubSurface& diffuser, const ConstructionBase& construction, double diameter,
		double totalLength, double effectiveThermalResistance);

Addresses #4380, completing SDK support of Tubular Daylighting Devices (TDDs) #4382

Addresses #4380, completing SDK support of Tubular Daylighting Devices (TDDs) #4382

Conversation

joseph-robertson commented Jul 23, 2021 • edited by jmarrec Loading

Pull request overview

Pull Request Author

Review Checklist

jmarrec Aug 17, 2021

Choose a reason for hiding this comment

jmarrec Aug 18, 2021

Choose a reason for hiding this comment

jmarrec Aug 18, 2021

Choose a reason for hiding this comment

jmarrec commented Aug 18, 2021

ci-commercialbuildings commented Aug 18, 2021 • edited Loading

jmarrec commented Aug 18, 2021

joseph-robertson commented Jul 23, 2021 •

edited by jmarrec

Loading

ci-commercialbuildings commented Aug 18, 2021 •

edited

Loading