Add Kiva foundation heat transfer calculations #5901

nealkruis · 2016-10-05T17:30:41Z

Pull request overview

Integration of Kiva into EnergyPlus as described in this NFP.

Work completed for:

Task Order No. KAGN-4-42502-04 under Task Ordering Agreement No. KAGN-4-42502-00: "2016 EnergyPlus Foundation Heat Transfer Algorithm Evaluation and Model Development" and
Subcontract AEU-7-70112-01: “2017 EnergyPlus Foundation Heat Transfer Algorithm Model Development”

Work Checklist

Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
- Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
- Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
- NewFeature: This pull request includes code to add a new feature to EnergyPlus
- Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
- DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

mbadams5

@nealkruis This is my first pass at an (impromptu) review. I may have other comments later. You may want to check indention and coding style to make sure it follows EnergyPlus guidelines.

mbadams5 · 2016-10-12T18:40:29Z

src/EnergyPlus/HeatBalanceKivaManager.cc

+      if (wallSurfaces.size() != 0) {
+        wallHeight = Surfaces(wallSurfaces[0]).Height; // TODO Kiva: each wall with different height gets its own instance.
+        for (auto& wl : wallSurfaces) {
+          if (Surfaces(wl).Height != wallHeight) {


@nealkruis Why not put this conditional check in the same loop on line 218? That would eliminate searching all wall surfaces a second time.

mbadams5 · 2016-10-12T18:43:19Z

src/EnergyPlus/HeatBalanceKivaManager.cc

+
+      // polygon
+      if (DataSurfaces::CCW) {
+        for (size_t i = 0; i < surface.Vertex.size_; ++i ) {


Why use surface.Vertex.size_ instead of surface.Vertex.size()? The latter seems more standard with STL since i'd imagine the size member variable would normally be private.

Do objexx arrays have a size method?

mbadams5 · 2016-10-12T18:46:56Z

src/EnergyPlus/HeatBalanceKivaManager.cc

+
+    // TODO Kiva: Add wall surfaces to EIO
+    gio::write( DataGlobals::OutputFileInits, "(A)" ) << "! <Kiva Foundation Name>, Horizontal Cells, Vertical Cells, Total Cells, Floor Surface, Wall Surface(s)";
+		gio::write( DataGlobals::OutputFileInits, "(A,',',I5',',I5',',I5',',A)" ) << foundationInputs[DataSurfaces::Surface(kv.floorSurface).OSCPtr].name << grnd.nX << grnd.nZ << grnd.nX*grnd.nZ << DataSurfaces::Surface(kv.floorSurface).Name;


Misaligned indention.

mbadams5 · 2016-10-12T18:49:16Z

src/EnergyPlus/HeatBalanceKivaManager.cc

+  if (DataEnvironment::Month == 1 && DataEnvironment::DayOfMonth == 2 && DataGlobals::HourOfDay == 2 && DataGlobals::TimeStep == 1) {
+    Kiva::SnapshotSettings ss;
+    ss.dir = DataStringGlobals::outDirPathName + "/snapshot";
+    double& l = ground.foundation.reductionLength2;


Why get the value by reference? You don't seem to be updating it anywhere. You do this in quite a few places...

Is there anything wrong with this? I'm using "l" as an alias so I don't have to type out "ground.foundation.reductionLength2" everywhere.

mbadams5 · 2016-10-12T18:51:00Z

src/EnergyPlus/HeatBalanceKivaManager.cc

+  Kiva::Foundation defFnd;
+
+  defFnd.hasWall = true;
+  defFnd.hasWall = true;


assigned twice, same for two below.

mbadams5 · 2016-10-12T19:27:37Z

third_party/cmake/kiva.cmake

+  include_directories( SYSTEM "${CMAKE_BINARY_DIR}/third_party/kiva/vendor/mathgl-2.3.5.1/include/")
+  add_subdirectory("${CMAKE_SOURCE_DIR}/third_party/kiva/vendor/zlib-1.2.8/")
+  add_subdirectory("${CMAKE_SOURCE_DIR}/third_party/kiva/vendor/libpng-1.6.23/")
+  add_subdirectory("${CMAKE_SOURCE_DIR}/third_party/kiva/vendor/mathgl-2.3.5.1/")


We need to make sure MathGL has a compatible license with EnergyPlus since it can be either GPL or LGPL. @jasondegraw

MathGL is used only for debugging purposes. It won't be in the final release.

mbadams5 · 2016-10-12T19:30:09Z

third_party/FMI/CMakeLists.txt

@@ -1,8 +1,9 @@

 INCLUDE_DIRECTORIES( "${CMAKE_SOURCE_DIR}/third_party/Expat/lib")
 INCLUDE_DIRECTORIES( "${CMAKE_SOURCE_DIR}/third_party/")
+INCLUDE_DIRECTORIES( ${CMAKE_SOURCE_DIR}/third_party/zlib )


Why include this now? It seemed to be working before? I'm just curious.

I moved it from the top CMakeLists.txt file. It is only used in the FMI/FMU stuff and conflicted with the zlib I'm using for the ground plot library.

mbadams5 · 2016-10-12T19:32:15Z

src/EnergyPlus/WeatherManager.cc

+		int count = 0;
+		std::string WeatherDataLine;
+
+		while (! ReadStatus) {


I'm surprised this can't be part of the existing file read somewhere else. It seems like we might be reading the weather file twice now, correct? Is AnnualAverageDrybulbTemp the only useful statistic?

I think there is a whole lot of information we can gather from one pass of the weather file instead of asking the users to input weather information. I'm starting with annual temperature, but this will likely expand to other purposes as well.

There are other places that are currently reading the .stat file or requiring user input which all could get the info directly from a pre-read of the epw file (water mains temperature, adaptive comfort values, FCfactor ground temps, other ground models). So, this is something good to get in place in some way or another.

mbadams5 · 2016-10-12T19:33:58Z

src/EnergyPlus/SurfaceGeometry.cc

 				} else if ( SameString( cAlphaArgs( ArgPointer ), "OtherSideConditionsModel" ) ) {
 					Found = FindItemInList( SurfaceTmp( SurfNum ).ExtBoundCondName, OSCM, TotOSCM );
 					if ( Found == 0 ) {
-						ShowSevereError( cCurrentModuleObject + "=\"" + SurfaceTmp( SurfNum ).Name + "\", invalid " + cAlphaFieldNames( ArgPointer ) + "=\"" + cAlphaArgs( ArgPointer ) + "\"." );
+						ShowSevereError( cCurrentModuleObject + "=\"" + SurfaceTmp( SurfNum ).Name + "\", invalid " + cAlphaFieldNames( ArgPointer + 1 ) + "=\"" + cAlphaArgs( ArgPointer + 1 ) + "\"." );


Was this a bug? Why the ArgPointer + 1?

It was a small bug that I fixed while I was writing the similar line above.

mbadams5 · 2016-10-12T19:43:45Z

src/EnergyPlus/HeatBalanceKivaManager.cc

+  defaultFoundation.foundation = defFnd;
+  defaultFoundation.name = "<Default Foundation>";
+
+  foundationInputs.push_back(defaultFoundation);


Where do you add other foundations other than default foundation? I can't seem to find any other spots, especially since you have a function to find a foundation by name.

This is coming...

EnergyArchmage · 2016-11-10T14:39:29Z

What is the status of the problem of warm up for design days, discussed in the NFP here?

https://gist.github.com/nealkruis/fde60b564425e1b10a5d3a8ec179df51#warm-up-and-design-days

Most EnergyPlus models in practice are autosizing the HVAC system based on building loads. If a ground model cannot be used for design days it will not be of much practical use. I hope this can be addressed somehow.

nealkruis · 2016-11-15T16:21:51Z

@EnergyArchmage The current plan is to use a steady-state initialization for design days. It is a conservative estimate of load through foundations since the actual peak load will likely be damped and lagged relative to the design day.

EnergyArchmage · 2016-11-16T17:20:41Z

@nealkruis So does that mean a long soak at design day conditions or a multi-year steady periodic description for a year?

Current sizing design practice, using the "Ground" boundary condition with Site:GroundTemperature:BuildingSurface filled by slab.exe, essentially uses a year descripton of the ground condition, as it would develop over many years. The date of the design day indicates when within the year the load calculations are being done at and it just uses the ground boundary condition for that date, well the month anyway.

So I think the precendent is to do sizing with annual-type ground conditions and I fear doing something very different is risky without some study of the implications on sizing results.

nealkruis · 2016-11-16T19:00:35Z

I would prefer to build up a better thermal history. Kiva has an effective method to do so, but there are a couple of EnergyPlus limitations we'd have to get around:

Much of the thermal history of the ground will be erased by the repeating warmup day anyway (we can possibly override the ground temperatures after warmup?)
Warmup of the ground independent of the building simulation would require a separate read of the weather file. I've already made some changes to enable this. However, there are still cases where a building is simulated with only design days and no weather file is defined.
Warmup of the ground requires an assumption of the adjacent zone temperature, which may not be controlled--i.e. setpoints aren't always reliable assumptions.

nrel-bot-3 · 2016-12-01T00:53:44Z

@nealkruis @lgentile it has been 14 days since this pull request was last updated.

mjwitte · 2017-02-16T22:21:19Z

Whoa, and over 5000 new files. I thought based on the NFP that we would be linking in a Kiva library "The computational core of Kiva can be compiled as an independent library and linked to the EnergyPlus executable." So, where's all the baggage from?

nealkruis · 2017-02-16T22:48:46Z

@Myoldmopar I've been looking into ground init stuff today and making some progress. I don't think I'll have something figured out in time for interface freeze, so if anything changes it'll be behind the scenes. I'll try to address the formatting and remaining @mbadams5 comments today or tomorrow.

@mjwitte word from above was to vendor Kiva instead of an using an external clone, hence the additional files (most of them are third(fourth?)-party boost geometry header files). I'd love to replace boost geometry with something slimmer, but that option was ruled out in discussions. We can chat privately if you'd like more details.

mjwitte · 2017-02-16T22:51:04Z

@nealkruis OK. That's all I need to know. I'll yield to @Myoldmopar and @jasondegraw on that front. BTW, this branch has some small conflicts.

mjwitte

@nealkruis Just a few comments/question mainly on the IDD.

mjwitte · 2017-02-16T23:05:24Z

idd/Energy+.idd.in

@@ -16057,6 +16064,372 @@ SurfaceProperty:OtherSideConditionsModel,
      \note  in contact with GroundDomain objects
      \default GapConvectionRadiation

+Foundation:Kiva,
+       \memo Refined definition of the foundation surface construction used to
+       \memo infrom two-diemensional heat transfer calculated using the Kiva


mjwitte · 2017-02-16T23:08:30Z

idd/Energy+.idd.in

+       \note Extent of insulation as measured from the wall interior
+       \units m
+       \type real
+       \minimum> 0.0


This cannot be zero? Same question for N3, N6, and N9.

If a material is assigned, a zero value would imply no insulation. If a material is not assigned, a zero value is implicit. Do you think allowing zero would be better? I already have an error if a value (including zero) is defined here and no material is defined.

It's a fine point. If someone wants to run some parametrics, the way it currently is, you'd have to change both fields in order to add or remove the insulation. Instead of having the material name there and simply running this value from zero to whatever. When you say error here, do you mean a warning or fatals out? Again, it's a fine point.

Yes it fatals out...though of course a value of zero won't even get past the input processor.

I don't have a strong opinion on this. I see you point, but there are also many places in the program where you can't use a value of zero (e.g., material thickness) and requires much more than changing two fields.

I can probably change this in about 20 min. So let me know if you think it's worthwhile.

Nah, just let it ride. If it causes frustrations, then we'll hear about it later.

Oh, but you should make it clear here with a \note, that if the material name is blank, then this field must also be blank. And in the I/O Ref if it's not there already. Same for similar fields.

I misspoke (mistyped?). It gives only a warning if you provide values without specifying a material.

mjwitte · 2017-02-16T23:13:08Z

idd/Energy+.idd.in

+
+Foundation:Kiva:Settings,
+       \memo Settings applied across all Kiva foundation calculations.
+       \memo Object is not required. If not defined, defaults will applied.


. . . defaults will be applied.

mjwitte · 2017-02-16T23:15:16Z

idd/Energy+.idd.in

+       \key ZeroFlux
+       \key GroundWater
+       \key Autocalculate
+       \default Autocalculate


lower case on Autocalculate. (EnergyPlus doesn't care, but other IDD validators do.)

The key, the default, or both?

Oh - this is a key choice "Autocalculate", not an autocalculatable field. This would be the only place in the entire IDD with "\key Autocalculate". I guess there's no rule against using it, but "autocalculate" does have special meaning in numeric fields. Is there a different word that would convey the same intent?

Just "Auto"?

Sure, or "Autoselect"? It's really not calculating a value here, it's just making a choice between the two models if I'm following the docs correctly.

mjwitte · 2017-02-16T23:23:46Z

src/EnergyPlus/HeatBalanceSurfaceManager.cc

+					SetupOutputVariable( "Surface Heat Storage Loss Rate [W]", OpaqSurfStorageCondLossRep( loop ), "Zone", "State", Surface( loop ).Name );
+					SetupOutputVariable( "Surface Heat Storage Rate per Area [W/m2]", OpaqSurfStorageConductionFlux( loop ), "Zone", "State", Surface( loop ).Name );
+					SetupOutputVariable( "Surface Heat Storage Energy [J]", OpaqSurfStorageConductionEnergy( loop ), "Zone", "Sum", Surface( loop ).Name );
+				}


Remind me (briefly) how the Kiva model connects to the surface and why these output can't be calculated?

Kiva simulates both the floor and the walls in the same two-dimensional model and they can exchange heat with deep ground, or the exterior grade. Heat stored in the ground can be come from any boundary. There are no set of "inside face" and "outside face" connected with the ground that can be used to calculate the storage for a specific surface.

Ok, got it, I fogot the surfaces are included in the Kiva domain. But the usual Inside Face outputs (temperature and flux) will still be available and calculated appropriately?

Yes. These are demonstrated in the example files.

Conflicts: .decent_ci-Linux.yaml

mjwitte · 2017-02-20T16:47:15Z

@nealkruis FYI - I merged in latest develop to get a clean set of CI tests, and resolved minor conflict in yaml file. Put the --suppress="*:*eigen*" at the end of the line after the new re2 flag.

mjwitte · 2017-02-20T21:38:28Z

@nealkruis This is showing -8% performance improvement. What's changing that would impact runs that don't use the kiva model?

nealkruis · 2017-02-20T22:02:24Z

@mjwitte It's likely the SetWeatherStatistics() call that reads the weather file to set AverageAnnualDrybulbTemp. I can probably pull this into the Kiva manager to avoid the extra read for every simulation. I'll need to do it again for initialization anyway. We might want to consider reading the whole weather file once into memory to improve capability without increasing I/O time.

mjwitte · 2017-02-20T22:06:48Z

@nealkruis Let's move it for now and then after this release, we can consider an initial read into memory for runs that use the weather file.

Myoldmopar · 2017-02-22T14:52:41Z

Appears there are still lots of CI problems on this branch.

Note we are five days past feature freeze.

nealkruis · 2017-02-22T16:44:09Z

@Myoldmopar I think the CI problems on Linux are the same we were seeing earlier--something screwy with the Linux machine. I'll double check locally, but I don't think it is a problem with the branch.

mbadams5 · 2017-02-22T17:30:34Z

@nealkruis It is a problem with kiva...

Linking CXX shared library ../../Products/libenergyplusapi.so
/usr/bin/ld: ../../Products/libkiva.a(Foundation.cpp.o): relocation R_X86_64_32 against `.rodata.str1.8' can not be used when making a shared object; recompile with -fPIC
../../Products/libkiva.a: error adding symbols: Bad value
collect2: error: ld returned 1 exit status
make[2]: *** [Products/libenergyplusapi.so.8.7.0] Error 1
make[1]: *** [src/EnergyPlus/CMakeFiles/energyplusapi.dir/all] Error 2
make: *** [all] Error 2

This is the same issue I had with re2 before, basically you need to compile on linux with -fPIC

nealkruis · 2017-02-22T17:36:30Z

Yep, just seeing this now. Thanks @mbadams5

mjwitte · 2017-02-22T19:56:59Z

@nealkruis Going to wait for CI to catch up to the latest commit on develop. If that's clean, then will re-merge develop here and wait for CI again.

Myoldmopar · 2017-02-23T02:49:26Z

Not sure it's worth another pass here. The warnings and failures are from other known issues. This should be good as far as i can tell from my mobile here.

mjwitte · 2017-02-23T02:54:39Z

@Myoldmopar That's your call. Develop is looking clean now, so I was going to merge develop in here shortly and let it run overnight.

mjwitte · 2017-02-23T03:19:51Z

@Myoldmopar There's a problem in the IDD. I'll fix it here.

nmerket · 2017-02-23T15:07:28Z

Wahoo! On Feb 23, 2017, at 7:45 AM, Michael J. Witte <notifications@github.com<mailto:notifications@github.com>> wrote: Merged #5901<#5901>. - You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#5901 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFFA6sW5wSk3y1MW8E3hCpotmQGFiWqRks5rfZuPgaJpZM4KPE60>.

nealkruis added 7 commits August 17, 2016 12:41

Link EnergyPlus to Kiva.

6f421d7

Initial Kiva integration files.

d616bac

Add annual average drybulb calculation from weather file.

fe3608d

Initial integration of floor slab.

4cf800d

Merge develop.

aa0906a

Improve output reporting.

a360112

Use latest from 'energyplus' Kiva branch.

87086af

nealkruis added the NewFeature Includes code to add a new feature to EnergyPlus label Oct 5, 2016

nealkruis added this to the EnergyPlus 8.7.0 milestone Oct 5, 2016

nealkruis self-assigned this Oct 5, 2016

nealkruis added 2 commits October 5, 2016 20:23

Fix gcc builds.

0f14ec7

Simulate single zone basements in Kiva.

546e826

mbadams5 reviewed Oct 12, 2016

View reviewed changes

nealkruis added 12 commits October 12, 2016 18:18

Fix zlib includes.

efcb011

Hookup IDF inputs.

0ee9929

Fix redeclaration.

b076a3f

Fix divide by zero.

6de0b39

Make horizontal insultaion depth relative.

4cc2c54

Merge branch 'kiva' of https://github.com/NREL/EnergyPlus into kiva

8a8c0ad

Add general material blocks to foundation definition.

30fc1f2

Change IDD language.

52a4a95

Hook up exposed perimeter object.

f939de7

Minor fix for EIO output.

a35d721

Misc. fixes.

8c42d64

Final fixes for prototype branch.

2f89682

mjwitte reviewed Feb 16, 2017

View reviewed changes

nealkruis and others added 3 commits February 16, 2017 22:11

Cleanup formatting, minor changes in response to review comments.

21f1ca5

Merge develop into kiva.

4735da9

Merge remote-tracking branch 'remotes/origin/develop' into kiva

243b65a

Conflicts: .decent_ci-Linux.yaml

nealkruis added 3 commits February 21, 2017 08:57

Make option to build libkiva as static or shared.

62ed336

Move weather file read into KivaManager.

2d7a254

Fix libkiva static build.

d656394

Merge branch 'develop' into kiva

47993d8

Fix Linux compilation with -fPIC.

0daec6a

Merge remote-tracking branch 'remotes/origin/develop' into kiva

ebef407

IDD cleanup

d99d020

mjwitte merged commit ae053ca into develop Feb 23, 2017

mjwitte deleted the kiva branch February 23, 2017 20:39

chriswmackey mentioned this pull request Aug 17, 2017

Make the Default EnergyPlus Ground Condition Calculated by KIVA ladybug-tools/honeybee-legacy#650

Open

Add Kiva foundation heat transfer calculations #5901

Add Kiva foundation heat transfer calculations #5901

Conversation

nealkruis commented Oct 5, 2016 • edited by mjwitte Loading

Pull request overview

Work Checklist

Review Checklist

mbadams5 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnergyArchmage commented Nov 10, 2016

nealkruis commented Nov 15, 2016

EnergyArchmage commented Nov 16, 2016

nealkruis commented Nov 16, 2016

nrel-bot-3 commented Dec 1, 2016

mjwitte commented Feb 16, 2017

nealkruis commented Feb 16, 2017

mjwitte commented Feb 16, 2017

mjwitte 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

mjwitte Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

nealkruis Feb 17, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjwitte commented Feb 20, 2017

mjwitte commented Feb 20, 2017

nealkruis commented Feb 20, 2017

mjwitte commented Feb 20, 2017 • edited Loading

Myoldmopar commented Feb 22, 2017

nealkruis commented Feb 22, 2017 • edited Loading

mbadams5 commented Feb 22, 2017

nealkruis commented Feb 22, 2017

mjwitte commented Feb 22, 2017

Myoldmopar commented Feb 23, 2017

mjwitte commented Feb 23, 2017

mjwitte commented Feb 23, 2017

nmerket commented Feb 23, 2017 via email

nealkruis commented Oct 5, 2016 •

edited by mjwitte

Loading

mjwitte Feb 17, 2017 •

edited

Loading

nealkruis Feb 17, 2017 •

edited

Loading

mjwitte commented Feb 20, 2017 •

edited

Loading

nealkruis commented Feb 22, 2017 •

edited

Loading