Skip to content

Surf heat balance arrays naming and initialization#8820

Merged
mjwitte merged 18 commits intoNatLabRockies:developfrom
energy-plus:surf-hb-struct-2
Jun 24, 2021
Merged

Surf heat balance arrays naming and initialization#8820
mjwitte merged 18 commits intoNatLabRockies:developfrom
energy-plus:surf-hb-struct-2

Conversation

@xuanluo113
Copy link
Contributor

@xuanluo113 xuanluo113 commented Jun 14, 2021

Pull request overview

The branch includes a lot of renaming and relocating surface heat balance arrays in DataSurface, DataHeatBalance and DataHeatBalSurface. Other minor changes are mostly in SolarShading.cc, including:

  1. Reordering 3D arrays from (TimeStep, Hour, SurfNum) to (Hour, TimeStep, Surfnum) as the arrays are accessed by first Hour and then Timesteps (e.g. SurfSunlitFrac, SurfCosIncAng, SurfWinBackSurfaces).
  2. Convert 3D array SUNCOSTS and SurfSunCosHourly to 2D of Vector3 arrays to avoid slicing and reconstructions.
  3. Convert some implicit zero assignment of arrays to explicit loop assignment.
  4. Reduce the scope of Encl only arrays from Zone to Enclosure.

Pull Request Author

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)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • 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
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@xuanluo113 xuanluo113 self-assigned this Jun 18, 2021
@xuanluo113 xuanluo113 added the Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus label Jun 18, 2021
@xuanluo113 xuanluo113 added this to the EnergyPlus 9.6 IOFreeze milestone Jun 18, 2021
@xuanluo113 xuanluo113 changed the title Surf hb struct Surf heat balance arrays naming and initialization Jun 21, 2021
@xuanluo113 xuanluo113 requested a review from mjwitte June 21, 2021 22:13
@xuanluo113
Copy link
Contributor Author

@mjwitte This PR is ready for review as a follow-up of the last surface HB struct branch. However, I'm puzzled by the 0.064% performance done CI showed. Again, tested and profiled locally, the run time of this branch doe not show a consistent speed up and down comparing with the develop branch. The run time fluctuation between two runs is way larger than the CI performance diff and it's hard to tell.

@amirroth
Copy link
Collaborator

Are you expecting any performance changes here? This looks mostly like naming conventions. I would ignore any fluctuations in the ranges you are looking at.

Copy link
Contributor

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A few comments.

where

\emph{FractDifShortZtoZ(OtherZoneNum,ZoneNum)} = ``diffuse solar exchange factor'' = fraction of short-wave radiation in \emph{OtherZoneNum} that is transmitted to \emph{ZoneNum}. This factor is calculated in subroutine ComputeDifSolExcZonesWIZWindows taking into account multiple reflection between zones. For example, for two zones means that some of the radiation transmitted from Zone1 to Zone2 is reflected back to Zone1, and some of this is in turn reflected back to Zone2, etc.
\emph{ZoneFractDifShortZtoZ(OtherZoneNum,ZoneNum)} = ``diffuse solar exchange factor'' = fraction of short-wave radiation in \emph{OtherZoneNum} that is transmitted to \emph{ZoneNum}. This factor is calculated in subroutine ComputeDifSolExcZonesWIZWindows taking into account multiple reflection between zones. For example, for two zones means that some of the radiation transmitted from Zone1 to Zone2 is reflected back to Zone1, and some of this is in turn reflected back to Zone2, etc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These may change with the addition of Spaces. And probably should be enclosure, but that's for the final frontier explorer to work out.

SumHATsurf += state.dataHeatBal->HConvIn(SurfNum) * state.dataSurface->SurfWinFrameArea(SurfNum) *
(1.0 + state.dataSurface->SurfWinProjCorrFrIn(SurfNum)) * state.dataSurface->SurfWinFrameTempSurfIn(SurfNum);
SumHATsurf += state.dataHeatBalSurf->SurfHConvInt(SurfNum) * state.dataSurface->SurfWinFrameArea(SurfNum) *
(1.0 + state.dataSurface->SurfWinProjCorrFrIn(SurfNum)) * state.dataSurface->SurfWinFramerTempIn(SurfNum);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SurfWinFramerTempIn should be SurfWinFrameTempIn.

Array3D<Real64> SUNCOSTS = Array3D<Real64>(60, 24, 3); // Timestep values of solar direction cosines
Array2D<Real64> BSDFTempMtrx; // Temporary matrix for holding axisymmetric input
EPVector<DataBSDFWindow::BSDFWindowGeomDescr> ComplexWind; // Window geometry structure: set in CalcPerSolarBeam/SolarShading
Array2D<Vector3<Real64>> SUNCOSTS = Array2D<Vector3<Real64>>(60, 24); // Timestep values of solar direction cosines
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooray! 1 less Array3D.


// Integer Variables for the Heat Balance Simulation
Array1D_int SUMH; // From Old Bldctf.inc
Array1D_int SurfCurrNumHist; // From Old Bldctf.inc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting name change - with much clearer meaning.

}

// Flux of diffuse solar in each zone

state.dataHeatBal->EnclSolQSDifSol = 0.0;
// state.dataHeatBal->EnclSolQSDifSol = 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete?

@@ -3558,19 +3557,20 @@ void InitIntSolarDistribution(EnergyPlusData &state)

for (int enclosureNum = 1; enclosureNum <= state.dataViewFactor->NumOfSolarEnclosures; ++enclosureNum) {

if (state.dataHeatBalSurf->RecDifShortFromZ(enclosureNum)) {
if (state.dataHeatBalSurf->ZoneRecDifShortFromZ(enclosureNum)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, here it seems clear that ZoneRecDifShortFromZ is an enclosure variable, I think?

}
}

void DetermineMaxBackSurfaces(EnergyPlusData &state)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that this function was present but never used ever since BSDF was added.

@xuanluo113
Copy link
Contributor Author

@mjwitte Thanks for the review! Comments addressed.

@mjwitte mjwitte marked this pull request as ready for review June 24, 2021 22:07
Copy link
Contributor

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Merging.

@mjwitte mjwitte merged commit 1a13147 into NatLabRockies:develop Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants