Automatic Import of Shading from a CSV File by xuanluo113 · Pull Request #6388 · NatLabRockies/EnergyPlus

xuanluo113 · 2017-11-09T02:08:19Z

Pull request overview

EnergyPlus version 8.8 added features to export the calculated shading results to an external CSV file, and allow to use shading schedules to replace internal shading calculations for exterior surfaces.
Although the shading schedules can be imported from external CSV files, there needs to define all these schedules in IDF files explicitly as well as adding SurfaceProperty:LocalEnvironment objects for all exterior surfaces, which can be a significant burden for users especially for a model with many exterior surfaces. The proposed new feature will fully automate the process.

Work Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

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.

nealkruis · 2017-11-09T15:02:55Z

@XuanLuoLBL @hongtz68

I think we should have a brainstorm on the best file format for caching shading results and consider something that can be transferred between tools (e.g., importing shading calculations performed in an external tool). CSV is easy to understand, but it is not as easy to standardize. Maybe something like JSON/CBOR that can be validated against a schema.
I assume the proposed time series values are intended to represent shading of direct beam radiation. What about the values of diffuse shading from the various grid points in the sky dome?

xuanluo113 · 2017-11-17T18:41:49Z

@nealkruis

CSV files are used in EnergyPlus to import schedules. We prefer to using the same approach. In future other formats can be added as needed.
We are propoping to overwrite both direct and diffused solar gains using SunlitFrac:
For direct solar gain

// Incident direct (unreflected) beam
QRadSWOutIncidentBeam( SurfNum ) = BeamSolar * SunlitFrac( TimeStep, HourOfDay, SurfNum2 ) * CosInc;

For diffused solar gain:
https://www.energyplus.net/sites/default/files/docs/site_v8.3.0/EngineeringReference/05-Climate/index.html#shadowing-of-sky-diffuse-solar-radiation

Wondering if that's right.

nealkruis · 2017-11-20T18:53:08Z

@XuanLuoLBL. Yes, that is the right section for diffuse solar shading (though a more up-to-date version is here). In order to cache shading for a surface, you'll need:

direct solar shading fractions for each timestep and each calculation period (e.g., every 30 days). Potentially, one for each timestep of the year (~50,000 for 10 min. timesteps and daily calculations).
diffuse shading fractions for each grid in the sky model (24 azimuths x 6 altitudes). Note: these are independent of timestep

There's also an additional question about whether you'll need to handle exterior reflections (though I'd suggest continue performing those calculations internally).

For the large models where caching would be really beneficial, you'll potentially need to cache on the order of 52 million double precision values (1,000 exterior surfaces x 144 diffuse shading fractions + 1,000 surfaces x ~50,000 timestep solar fractions). I would like to have a better sense of the storage and IO read performance implications of different file storage formats (namely CSV vs. binary) for this magnitude of data.

Myoldmopar · 2017-11-29T01:51:39Z

@nealkruis For those huge cases, where you are talking on the order of 50 million values to be stored, the storage doesn't bother me too much. Users should understand that when they do big models, they will need storage. I guess we could try to warn them by estimating the size of the cached file and checking available file system capacity...

Processing that file makes me cringe a little, but not to the point of requiring a binary format. By using the cached shading values, the user should experience a major improvement in runtime compared to recalculation. Any additional gains from a switch to binary during reading should pale in comparison to the overall gain. And I really don't want EnergyPlus to have to manage our own binary files when it's not a necessary step.

My main concern is when you mentioned standardizing. @XuanLuoLBL, yeah E+ does currently read schedules from CSV. But we are trying to move forward in the world, with the impending JSON input format, and now the JSON-based G-Function caching for ground heat exchangers. My strong preference here would be to create a nicely structured JSON format for these values, such that we don't have to rely on a predetermined column order in a huge csv file during export/import. I do understand that raw JSON will carry some additional burden over CSV, but as long as we are smart with the JSON structure, the read time penalty should still be small compared to the overall massive gains of this project.

nrel-bot-3 · 2017-12-26T00:20:31Z

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

nrel-bot · 2018-01-21T00:50:06Z

@XuanLuoLBL @lgentile it has been 16 days since this pull request was last updated.

…oImportExtShading

nrel-bot · 2018-02-07T01:06:50Z

@XuanLuoLBL @lgentile it has been 17 days since this pull request was last updated.

Myoldmopar

@XuanLuoLBL This is pretty close, but I made a few comments that should be addressed. The main one is the JSON comments. I was hoping the code would be set up so that we could easily add JSON to this at a later date. It is added to the IDD and docs in this pull request, appearing to indicate it is already available and implemented, which I don't think it is. We should remove it from those user-facing interfaces until we actually have JSON available. If you can clean that up and address my other comments, this should be good to go in. Testing now...

Myoldmopar · 2018-02-13T14:27:00Z

idd/Energy+.idd.in

+  A2 ; \field File Format
+       \type choice
+       \key CSV
+       \key JSON


Wait, what? Can this do JSON?? If this is just a placeholder option for later that isn't implemented, then it needs to go away.

Yes. It's a placeholder as we discussed for the NFP. I can remove it.

During the NFP stage, I wanted the code to be written so that later it would be easy to add JSON. We can't have things in the IDD that the user sees that aren't true. Sorry for the confusion.

Myoldmopar · 2018-02-13T14:27:37Z

idd/Energy+.idd.in


+Schedule:File:Shading,
+       \min-fields 1
+       \memo A Schedule:File:Shading points to a text computer file that has


Probably want to reword this, don't need to say "computer file". And instead of text file, how about CSV file?

That is word for Schedule:File object and confused me as well... Modified.

Myoldmopar · 2018-02-13T14:28:23Z

idd/Energy+.idd.in

+Schedule:File:Shading,
+       \min-fields 1
+       \memo A Schedule:File:Shading points to a text computer file that has
+       \memo hourly/sub-hourly sunlit fraction data for all exterior surfaces.


It's not really for "all" exterior surfaces though, right? In the design/documentation it says that if surfaces are missing, they are simply not shaded.

@XuanLuoLBL If sub-hourly data are allowed, do you force fixed time interval step or allow flexible time interval. Please specify it clearly.

@lgu1234 Sorry not sub-hourly... Not necessary for sunlit fraction. Modified the text.

Myoldmopar · 2018-02-13T14:29:55Z

src/EnergyPlus/ScheduleManager.cc

+				ShowContinueError( "Try again with putting full path and file name in the field." );
+				ErrorsFound = true;
+			} else {
+				SchdFile = GetNewUnitNumber();


I know this is how it's done in other places of the code, but I wish I would've seen this earlier and directed to not use this approach. This file reading approach is simply a carryover from the Fortran days, and using C++ file streams would've been a nice thing to do instead. But it's a bit late now and not worth holding this up.

Okay. I wish I could see this earlier. I thought I should use familiar code to keep the same pattern, as we do everything else.

@XuanLuoLBL @Myoldmopar See #6482 followup issue to revise this.

Myoldmopar · 2018-02-13T14:31:34Z

src/EnergyPlus/ScheduleManager.cc

+				}
+			}
+		}
+


Yeah I didn't see in there where you actually added JSON. What I had requested previously was for the code to be set up so that once we added JSON, it would be very easy to modify this code to also read JSON inputs. But as the JSON world isn't in here yet, we need to remove the option from the IDD and docs. (Unless I somehow missed the JSON reading in here and it actually does work...)

No. Not ready. My misunderstanding. But yes here it is for when we can actually add JSON in the future.

Myoldmopar · 2018-02-13T14:32:34Z

src/EnergyPlus/ScheduleManager.hh


+// C++ Headers
+#include <map>
+


Was there a reason you needed to include this here? I don't see any of your changes needing it. If you need it in the .cc file, go ahead and put it over there, not in the .hh file.

Myoldmopar · 2018-02-13T14:44:05Z

src/EnergyPlus/ScheduleManager.cc

 		using DataSystemVariables::iUnicode_end;
 		using DataSystemVariables::TempFullFileName;
 		using DataSystemVariables::CheckForActualFileName;
+		using DataSystemVariables::UseImportedSunlitFrac;


This isn't being used in this function and is throwing the build warning. That will clean up CI some.

Myoldmopar · 2018-02-13T14:49:40Z

Regarding CI:

I commented about the build warning. That will be easy to fix.
The test failure is because your example file for importing scheduled shading is expecting to find those files next to the test file during the run, but CI doesn't know that it needs to bring that file along when it sets up the run folders. You will have to modify the CMake run script at cmake/ProjectMacros.cmake to bring in that external file dependency. It's a very lightweight file, so as a first pass, you could just add the CMake code to always bring it in, regardless of the file being tested. Then if that works, you could add a CMake option so it only does it for your particular file.

Also, I built locally, and just ran the ExternalShading example file, and my eplusshading.csv is empty. Is that normal?

lgu1234 · 2018-02-13T18:43:40Z

idd/Energy+.idd.in

+Schedule:File:Shading,
+       \min-fields 1
+       \memo A Schedule:File:Shading points to a text computer file that has
+       \memo hourly/sub-hourly sunlit fraction data for all exterior surfaces.


@XuanLuoLBL If sub-hourly data are allowed, do you force fixed time interval step or allow flexible time interval. Please specify it clearly.

lgu1234 · 2018-02-13T18:58:21Z

src/EnergyPlus/ScheduleManager.cc

+				// create string of which day of year
+				++iDay;
+				++hDay;
+				if ( iDay > 366 ) break;


@XuanLuoLBL Do you allow data with 6 month? For example, the run period is from Jan. 1 to Jun 60. Can I have a data file with hourly values in 6 months?

No... Not specified here. I added in the doc it has to be 8760-8764.

Myoldmopar · 2018-02-16T18:32:13Z

@XuanLuoLBL

@lgu1234 and myself made a number of requested changes to this branch 3 days ago. You have not responded to any of them. Considering we are so late, this is in danger of not making the cutoff unless you address them right away.

…oImportExtShading

xuanluo113 · 2018-02-18T20:28:11Z

@Myoldmopar I'm not very familiar with the this CI CMake. Would you mind using some more specific word for "bring in that external file dependency". I don't see any chunk does similar things.

xuanluo113 · 2018-02-18T22:17:03Z

The example file for this PR is _ImportShading. Not ExternalShading.

…ExtShading Conflicts: src/EnergyPlus/DataSystemVariables.cc src/EnergyPlus/DataSystemVariables.hh src/EnergyPlus/SolarShading.cc testfiles/CMakeLists.txt

mjwitte

This should be ready to merge if CI comes back clean.

mjwitte · 2018-02-19T22:02:54Z

src/EnergyPlus/ScheduleManager.cc

+				ShowContinueError( "Try again with putting full path and file name in the field." );
+				ErrorsFound = true;
+			} else {
+				SchdFile = GetNewUnitNumber();


@XuanLuoLBL @Myoldmopar See #6482 followup issue to revise this.

mjwitte · 2018-02-19T22:04:19Z

src/EnergyPlus/ScheduleManager.cc

+				ErrorsFound = true;
+			} else {
+				SchdFile = GetNewUnitNumber();
+				{ IOFlags flags; flags.ACTION( "read" ); gio::open( SchdFile, ShadingSunlitFracFileName, flags ); read_stat = flags.ios(); }


@XuanLuoLBL This needs to use TempFullFileName here because the file could be in a number of different places. I'll fix this.

mjwitte · 2018-02-20T00:57:37Z

This needs a clear_state (well, the other shading thing need it, but adding it here). Will push shortly. Then this should be ready to drop in.

Myoldmopar · 2018-02-20T00:58:51Z

👍 Thanks @mjwitte

Myoldmopar · 2018-02-20T01:06:43Z

And develop just moved with the other PRs. I considered saying just push your changes and we'll merge it but I think since clear_states can cause issues, we should let CI play this one all the way out. Make sure to pull develop before pushing and when CI is totally happy it can drop in.

…ExtShading

xuanluo113 · 2018-02-20T01:49:10Z

Thanks so much! @mjwitte

mjwitte · 2018-02-20T01:53:00Z

@XuanLuoLBL If you can, please review the file diffs on this PR. I had to resolve some merge conflicts after the other shading feature dropped in. It would be good if you can make sure I didn't break anything when resolving the conflicts.

xuanluo113 · 2018-02-20T01:55:50Z

@mjwitte Sure.

xuanluo113 · 2018-02-20T02:18:06Z

@mjwitte Seems good. I don't see anything missed conflict in IDD and code. All data sys vars are still there.

mjwitte · 2018-02-20T02:33:44Z

Thanks for checking. This will drop in once CI finishes.

mjwitte · 2018-02-20T05:16:02Z

Ack - the new test file is failing on linux debug. @Myoldmopar please run on linux and see what's up.

mjwitte · 2018-02-20T06:19:27Z

@Myoldmopar @XuanLuoLBL Found the problem - nan's when checking the file max/min. Leap year logic needs review - see #6482 . For now, data file lengthened to 8784 data rows.

Myoldmopar · 2018-02-20T11:39:32Z

All good now, merging.

NFP

1003ad6

mjwitte added NewFeature Includes code to add a new feature to EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels Dec 11, 2017

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into Aut…

521f115

…oImportExtShading

Code & test file & doc

64262ca

xuanluo113 self-assigned this Feb 7, 2018

xuanluo113 added 2 commits February 7, 2018 11:50

debug

10c0b1e

try again

16624e2

xuanluo113 requested review from Myoldmopar, lgu1234 and mjwitte February 11, 2018 20:39

Myoldmopar requested changes Feb 13, 2018

View reviewed changes

Myoldmopar reviewed Feb 13, 2018

View reviewed changes

lgu1234 reviewed Feb 13, 2018

View reviewed changes

Myoldmopar added this to the EnergyPlus 8.9.0 milestone Feb 14, 2018

Myoldmopar modified the milestones: EnergyPlus 8.9.0, EnergyPlus Next Feb 17, 2018

xuanluo113 added 2 commits February 18, 2018 12:19

debug

d45847d

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into Aut…

f55ec13

…oImportExtShading

mjwitte mentioned this pull request Feb 18, 2018

Additional Terminal Unit Sizing Issues #6481

Closed

11 tasks

mjwitte modified the milestones: EnergyPlus Next, EnergyPlus 8.9.0 Feb 19, 2018

mjwitte added 2 commits February 19, 2018 14:37

Merge remote-tracking branch 'remotes/origin/develop' into AutoImport…

a334207

…ExtShading Conflicts: src/EnergyPlus/DataSystemVariables.cc src/EnergyPlus/DataSystemVariables.hh src/EnergyPlus/SolarShading.cc testfiles/CMakeLists.txt

Shading import - rename data file and fix test failures

fdbb5db

mjwitte approved these changes Feb 19, 2018

View reviewed changes

Shading import - rename data file again and fix other test file

6681b3a

mjwitte added 2 commits February 19, 2018 19:13

Merge remote-tracking branch 'remotes/origin/develop' into AutoImport…

378c380

…ExtShading

Add clear_state for DataSystemVariables

8b446b9

Shading import - lengthen data file to workaround test failure

a83d7bb

Shading import - add unique-object

748b65e

Myoldmopar merged commit 545c141 into develop Feb 20, 2018

Myoldmopar deleted the AutoImportExtShading branch February 20, 2018 11:39

aaron-boranian mentioned this pull request Mar 14, 2018

Reuse shading calculations between runs / implement external shading calculations #6528

Closed

aaron-boranian mentioned this pull request Mar 30, 2018

Integrate Window Calculation Engine #6673

Closed

Conversation

xuanluo113 commented Nov 9, 2017

Pull request overview

Work Checklist

Review Checklist

Uh oh!

nealkruis commented Nov 9, 2017

Uh oh!

xuanluo113 commented Nov 17, 2017

Uh oh!

nealkruis commented Nov 20, 2017

Uh oh!

Myoldmopar commented Nov 29, 2017

Uh oh!

nrel-bot-3 commented Dec 26, 2017

Uh oh!

nrel-bot commented Jan 21, 2018

Uh oh!

nrel-bot commented Feb 7, 2018

Uh oh!

Myoldmopar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Myoldmopar commented Feb 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Myoldmopar commented Feb 16, 2018

Uh oh!

xuanluo113 commented Feb 18, 2018

Uh oh!

xuanluo113 commented Feb 18, 2018

Uh oh!

mjwitte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjwitte commented Feb 20, 2018

Uh oh!