Wrap ZoneProperty:UserViewFactors:bySurfaceName object #3671

joseph-robertson · 2019-09-09T19:47:42Z

Addresses #3664.

Design doc: https://docs.google.com/document/d/1P8O6GRQAZda5kK_2Qu2CJRkBGUxTIhJwvJ0-JNpTU1A/edit#heading=h.c285d1mdhew5

TODO:

joseph-robertson · 2019-09-10T21:01:11Z

~~@jmarrec I tried testing RT in the app, and it didn't appear to be working. I'm currently writing a test for RT, but perhaps in the meantime you could take a look at the RT cpp file for this?~~

jmarrec · 2019-09-11T17:22:14Z

@joseph-robertson I can work on this tomorrow morning. What in the RT isnt working? Is it not translating the object at all or it's failing?

joseph-robertson · 2019-09-11T17:24:23Z

@jmarrec I got the RT working, so don't worry about that. This is ready for your review.

…ip trailing spaces

…t have at least one view factor

…will error out

…erty reference the same ThermalZone.

… allowing people to call direct Ctor for ZoneProp

jmarrec

This is great work @joseph-robertson, very thorough. I've made a few changes that I explained in the code as comments (some might be considered pedantic probably!), hopefully they make sense. I clicked "Resolved" on the ones I did address directly, so you'll have to expand them.

The only thing worth considering anymore is in this comment: #3671 (diff), especially the naming convention part. I am fine you just say it's better this way (hence why I set the status to "Approve")

(Out of curiosity: I have been wondering one thing: why was this chosen to be placed at the ThermalZone level and not the Space level? I honestly didn't take the time to carefully evaluate both options, and I assume you guys have)

...rc/energyplus/ForwardTranslator/ForwardTranslateZonePropertyUserViewFactorsBySurfaceName.cpp

openstudiocore/src/model/ZonePropertyUserViewFactorsBySurfaceName.cpp

openstudiocore/src/model/ZonePropertyUserViewFactorsBySurfaceName.hpp

jmarrec · 2019-09-12T07:38:08Z

openstudiocore/src/model/ZonePropertyUserViewFactorsBySurfaceName.hpp

+    ViewFactor(const InternalMass& fromInternalMass, const Surface& toSurface, double viewFactor);
+    ViewFactor(const InternalMass& fromInternalMass, const SubSurface& toSubSurface, double viewFactor);    
+
+    boost::optional<Surface> fromSurface() const;


Perhaps we should also add a ModelObject toSurfaceAsModelObject() const and a ModelObject fromSurfaceAsModelObject() const. You could then use that in the ostream overload to avoid testing all cases too, as well as in the addViewFactor(const ViewFactor&) and make your code dryer.
You'd probably also change the logic though, and use a single ModelObject m_fromSurface and ModelObject m_toSurface as members of the ViewFactor class.
(I do not feel very strongly about either design choice)

The naming conventions do not match the similar object SurfacePropertyConvectionCoefficients which uses:

ModelObject surfaceAsModelObject() const; boost::optional<Surface> surfaceAsSurface() const; boost::optional<SubSurface> surfaceAsSubSurface() const; boost::optional<InternalMass> surfaceAsInternalMass() const;

I think it might be beneficial to match this naming convention so our API is more predictable.

@joseph-robertson @shorowit Thoughts?

I agree with #2.

So we want

boost::optional<Surface> fromSurfaceAsSurface() const; boost::optional<SubSurface> fromSurfaceAsSubSurface() const; boost::optional<Surface> toSurfaceAsSurface() const; boost::optional<SubSurface> toSurfaceAsSubSurface() const;

etc?

Works for me.

openstudiocore/src/model/ZonePropertyUserViewFactorsBySurfaceName.cpp

...rc/energyplus/ForwardTranslator/ForwardTranslateZonePropertyUserViewFactorsBySurfaceName.cpp

openstudiocore/resources/model/OpenStudio.idd

...rc/energyplus/ReverseTranslator/ReverseTranslateZonePropertyUserViewFactorsBySurfaceName.cpp

openstudiocore/src/model/ZonePropertyUserViewFactorsBySurfaceName.hpp

…sibleGroup)

…message

jmarrec · 2019-09-12T14:26:21Z

openstudiocore/src/model/ModelGeometry.i

@@ -135,6 +135,15 @@ class ExteriorLoadInstance;
  }
 };

+%extend openstudio::model::ViewFactor {
+  // Use the overloaded operator<< for string representation


shorowit · 2019-09-12T14:48:39Z

(Out of curiosity: I have been wondering one thing: why was this chosen to be placed at the ThermalZone level and not the Space level? I honestly didn't take the time to carefully evaluate both options, and I assume you guys have)

In EnergyPlus, ultimately every combination of surfaces in a thermal zone needs a view factor. If you set view factors on a space level, you wouldn't be able to define view factors between two surfaces of different spaces within a given thermal zone. Granted these view factors would probably often be zero, but I'd hate to make OpenStudio more constrained than EnergyPlus for situations where the extra flexibility is desired.

jmarrec · 2019-09-12T15:15:21Z

(Out of curiosity: I have been wondering one thing: why was this chosen to be placed at the ThermalZone level and not the Space level? I honestly didn't take the time to carefully evaluate both options, and I assume you guys have)

In EnergyPlus, ultimately every combination of surface in a thermal zone needs a view factor. If you set view factors on a space level, you wouldn't be able to define view factors between two surfaces of different spaces within a given thermal zone. Granted these view factors would probably often be zero, but I'd hate to make OpenStudio more constrained than EnergyPlus for situations where the extra flexibility is desired.

That was one thing I noticed, right now there's no checks whatsoever that are done to ensure that the to/from objects are actually part of the thermal zone referenced. I didn't bring it up because it's probably not going to be used much by lambda users.

shorowit · 2019-09-12T15:18:29Z

That really ought to be added -- checks in the addViewFactor methods that both the from/to surfaces are in the thermal zone that the view factors are being added for.

shorowit · 2019-09-12T20:10:06Z

Are there checks if someone adds two view factors for the same combination of surfaces?

joseph-robertson · 2019-09-12T20:38:10Z

Sure aren't. I'll see if I can add.

…dd ALL combinations (numSurfaces^2) Follow up #79

jmarrec · 2019-09-13T10:44:54Z

Closing this PR as superseded by #3675 (we diverged at some point and I wanted to avoid force pushing)

Initial implementation of zone property user view factors.

d3f88d8

joseph-robertson self-assigned this Sep 9, 2019

joseph-robertson added 3 commits September 9, 2019 16:22

Simplify view factor data methods by using planar surface.

e0506de

Update all files; now building and passing.

da957a8

Forgot to swig the new viewfactor class.

ac47e6f

joseph-robertson mentioned this pull request Sep 10, 2019

Wrap ZoneProperty:UserViewFactors:bySurfaceName object NREL/OpenStudio-resources#79

Merged

23 tasks

joseph-robertson requested a review from jmarrec September 10, 2019 20:50

RT of thermal zone returns a space.

4a3367b

joseph-robertson added this to the OpenStudio 2.9.0 milestone Sep 11, 2019

joseph-robertson added the Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. label Sep 11, 2019

joseph-robertson added the component - Model label Sep 11, 2019

jmarrec added 8 commits September 12, 2019 09:11

Merge branch 'develop' into user-view-factors

63204a0

Reorder members to avoid warning about Ctor initialization order, Str…

c34b583

…ip trailing spaces

Make a threadsafe ctor test by itself.

32325f6

Initialize booleans and add OS_ASSERT statements (for debug).

92765f1

Do not Forward transalte object if: Zone wasn't translated, if doesn'…

bf6c593

…t have at least one view factor

Wrong casing in include. GCC is more stringent than MSVC on this and …

6e7c130

…will error out

Uniqueness? Bare minimum is to issue a warning when multiple ZoneProp…

ef791a4

…erty reference the same ThermalZone.

Enforce uniqueness in the thermalZone => ZoneProp relationship, still…

ad495fd

… allowing people to call direct Ctor for ZoneProp

jmarrec approved these changes Sep 12, 2019

View reviewed changes

jmarrec added 3 commits September 12, 2019 15:54

Add a FT test, which makes OS crash hard

59074f3

Avoid crash, (+rely on ModelExtensibleGroup instead of WorkspaceExten…

c616d8a

…sibleGroup)

Unrelated: surfaceprop doesn't have a name, so don't use that in log …

6129791

…message

jmarrec reviewed Sep 12, 2019

View reviewed changes

Change viewfactor methods to be more consistent with api.

572bf0d

From and to objects must be attached to the zone.

a43c5a1

jmarrec added a commit to NREL/OpenStudio-resources that referenced this pull request Sep 13, 2019

Ref NREL/OpenStudio#3671: in order to avoid E+ warning, you need to a…

8808bd6

…dd ALL combinations (numSurfaces^2) Follow up #79

jmarrec mentioned this pull request Sep 13, 2019

Wrap ZoneProperty:UserViewFactors:bySurfaceName object #3675

Merged

12 tasks

jmarrec closed this Sep 13, 2019

jmarrec deleted the user-view-factors branch September 16, 2019 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap ZoneProperty:UserViewFactors:bySurfaceName object #3671

Wrap ZoneProperty:UserViewFactors:bySurfaceName object #3671

joseph-robertson commented Sep 9, 2019 •

edited

Loading

joseph-robertson commented Sep 10, 2019 •

edited

Loading

jmarrec commented Sep 11, 2019

joseph-robertson commented Sep 11, 2019

jmarrec left a comment •

edited

Loading

jmarrec Sep 12, 2019

shorowit Sep 12, 2019

joseph-robertson Sep 12, 2019

shorowit Sep 12, 2019

jmarrec Sep 12, 2019

shorowit commented Sep 12, 2019 •

edited

Loading

jmarrec commented Sep 12, 2019 •

edited

Loading

shorowit commented Sep 12, 2019

shorowit commented Sep 12, 2019

joseph-robertson commented Sep 12, 2019

jmarrec commented Sep 13, 2019

Wrap ZoneProperty:UserViewFactors:bySurfaceName object #3671

Wrap ZoneProperty:UserViewFactors:bySurfaceName object #3671

Conversation

joseph-robertson commented Sep 9, 2019 • edited Loading

joseph-robertson commented Sep 10, 2019 • edited Loading

jmarrec commented Sep 11, 2019

joseph-robertson commented Sep 11, 2019

jmarrec left a comment • edited Loading

Choose a reason for hiding this comment

jmarrec Sep 12, 2019

Choose a reason for hiding this comment

shorowit Sep 12, 2019

Choose a reason for hiding this comment

joseph-robertson Sep 12, 2019

Choose a reason for hiding this comment

shorowit Sep 12, 2019

Choose a reason for hiding this comment

jmarrec Sep 12, 2019

Choose a reason for hiding this comment

shorowit commented Sep 12, 2019 • edited Loading

jmarrec commented Sep 12, 2019 • edited Loading

shorowit commented Sep 12, 2019

shorowit commented Sep 12, 2019

joseph-robertson commented Sep 12, 2019

jmarrec commented Sep 13, 2019

joseph-robertson commented Sep 9, 2019 •

edited

Loading

joseph-robertson commented Sep 10, 2019 •

edited

Loading

jmarrec left a comment •

edited

Loading

shorowit commented Sep 12, 2019 •

edited

Loading

jmarrec commented Sep 12, 2019 •

edited

Loading