Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WindowManager Continued #10874

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

WindowManager Continued #10874

wants to merge 6 commits into from

Conversation

amirroth
Copy link
Collaborator

@amirroth amirroth commented Dec 24, 2024

This quick PR transitions all window arrays indexed by incident angle and polynomial coefficient from Array1D to std::array. In a following PR will move glass layer into std::array as well moving this module to C-based storage and indexing.

Copy link

⚠️ Regressions detected on macos-14 for commit e509a84

Regression Summary
  • ESO Small Diffs: 651
  • EIO: 577
  • MTR Small Diffs: 541
  • Table Small Diffs: 462
  • Table String Diffs: 196
  • ZSZ Small Diffs: 63
  • JSON Big Diffs: 3
  • Table Big Diffs: 24
  • ERR: 18
  • EDD: 5
  • MTR Big Diffs: 2
  • ESO Big Diffs: 8
  • SSZ Small Diffs: 14
  • DELightIn: 1
  • DELightOut: 1
  • JSON Small Diffs: 1

Copy link

⚠️ Regressions detected on macos-14 for commit ee9ecea

Regression Summary
  • ESO Small Diffs: 651
  • EIO: 577
  • MTR Small Diffs: 541
  • Table Small Diffs: 462
  • Table String Diffs: 196
  • ZSZ Small Diffs: 63
  • JSON Big Diffs: 3
  • Table Big Diffs: 24
  • ERR: 18
  • EDD: 5
  • MTR Big Diffs: 2
  • ESO Big Diffs: 8
  • SSZ Small Diffs: 14
  • DELightIn: 1
  • DELightOut: 1
  • JSON Small Diffs: 1

@@ -2025,18 +2025,6 @@ void ConstructionProps::setArraysBasedOnMaxSolidWinLayers(EnergyPlusData &state)
this->rbBareVisCoef.allocate(state.dataHeatBal->MaxSolidWinLayers);
this->afBareSolCoef.allocate(state.dataHeatBal->MaxSolidWinLayers);
this->abBareSolCoef.allocate(state.dataHeatBal->MaxSolidWinLayers);
for (int Layer = 1; Layer <= state.dataHeatBal->MaxSolidWinLayers; ++Layer) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't need to be allocated because they are std::arrays now.

@@ -216,32 +216,32 @@ namespace Construction {
Real64 AbsDiffShade = 0.0; // Diffuse solar absorptance for shade
Real64 AbsDiffBackShade = 0.0; // Diffuse back solar absorptance for shade
Real64 ShadeAbsorpThermal = 0.0; // Diffuse back thermal absorptance of shade
Array1D<Array1D<Real64>> AbsBeamCoef; // Coefficients of incidence-angle polynomial for solar
Array1D<std::array<Real64, DataSurfaces::MaxPolyCoeff>> AbsBeamCoef; // Coefficients of incidence-angle polynomial for solar
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Polynomial coefficients are now held in std::array because the maximum number is known at compile time.

@@ -82,6 +82,13 @@ namespace General {
Real64 X_0, // 1st bound of interval that contains the solution
Real64 X_1); // 2nd bound of interval that contains the solution

constexpr Real64 POLYF(Real64 const X, // Cosine of angle of incidence
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

std::array version of this function.

Array1D<Real64> tvisFit(10); // Fitted visible transmittance vs incidence angle
Array1D<Real64> rfsolFit(10); // Fitted solar front reflectance vs incidence angle
Array2D<Real64> solabsFit(5, 10); // Fitted solar absorptance vs incidence angle for each glass layer
Array1D<Real64> TsolTemp(Window::numPhis+1); // Solar transmittance vs incidence angle; diffuse trans.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need temporary Array1D's in order to read from the W5 input file. Can probably move these to local variables now that I think about it.

@@ -4134,18 +4138,6 @@ namespace HeatBalanceManager {
if (NextLine.eof) goto Label1000;
++FileLineCount;

// Pre-calculate constants
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cosPhis is now a constexpr std::array in the Window module header file.

@@ -75,9 +74,16 @@ namespace Window {

int constexpr maxGlassLayers = 5;
int constexpr maxGapLayers = 5;
int constexpr maxIncidentAngles = 20;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is never 20, number of elements always hardwired to 10 during use. So why allocate all the arrays to 20?

Real64 constexpr dPhiDeg = 10.0;
Real64 constexpr dPhiRad = dPhiDeg * Constant::DegToRad;

constexpr std::array<Real64, numPhis> cosPhis =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bake cos(phi) at 10.0 degree increments into the executable, no need to compute at runtime.

int N1, // First and last data points used
int N2,
Array1A<Real64> CoeffsCurve // Polynomial coeffients from fit
void W5LsqFit(std::array<Real64, numPhis> const &ivars, // Independent variables
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Version of this function using std::array. Array size and polynomial order arguments removed because they were always the same.

ShowFatalError(state, "Errors found getting inputs. Previous error(s) cause program termination.");
}

// Loop over incidence angle from 0 to 90 deg in 10 deg increments.
// Get glass layer properties, then glazing system properties (which include the
// effect of inter-reflection among glass layers) at each incidence angle.

for (int IPhi = 1; IPhi <= TotalIPhi; ++IPhi) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to calculate this anymore.

@@ -2545,121 +2517,6 @@ namespace Window {

//****************************************************************************

#ifdef GET_OUT
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This giant thing was not used.

@amirroth amirroth added this to the EnergyPlus 25.1 milestone Dec 25, 2024
@amirroth amirroth added DoNotPublish Includes changes that shouldn't be reported in the changelog Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring labels Dec 25, 2024
@amirroth
Copy link
Collaborator Author

⚠️ Regressions detected on macos-14 for commit ee9ecea

Regression Summary

These diffs make no sense. None of them are in windows related files. These almost look like they belong to a different PR.

@@ -642,27 +636,22 @@ namespace Window {
}
} // End of loop over glass layers in the construction for front calculation

if (TotalIPhi > maxIncidentAngles) {
if (TotalIPhi > numPhis) {
ShowSevereError(state,
format("WindowManage::InitGlassOpticalCalculations = {}, Invalid maximum value of common incident angles = {}.",
Copy link
Contributor

Choose a reason for hiding this comment

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

This check looks like it was intended for Window::OpticalDataModel::SpectralAndAngle but does that method even care how many Phis there are? Looks like it just accesses tables directly now. I'm pretty sure this if block can be removed. It became irrelevant in #6749 See comments.

Copy link

⚠️ Regressions detected on macos-14 for commit 5febbe5

Regression Summary
  • ESO Small Diffs: 651
  • EIO: 577
  • MTR Small Diffs: 541
  • Table Small Diffs: 462
  • Table String Diffs: 196
  • ZSZ Small Diffs: 63
  • JSON Big Diffs: 3
  • Table Big Diffs: 24
  • ERR: 18
  • EDD: 5
  • MTR Big Diffs: 2
  • ESO Big Diffs: 8
  • SSZ Small Diffs: 14
  • DELightIn: 1
  • DELightOut: 1
  • JSON Small Diffs: 1

Copy link

⚠️ Regressions detected on macos-14 for commit be9456e

Regression Summary
  • ESO Small Diffs: 650
  • MTR Small Diffs: 539
  • EIO: 555
  • Table Small Diffs: 442
  • Table String Diffs: 212
  • ZSZ Small Diffs: 50
  • JSON Big Diffs: 3
  • Table Big Diffs: 27
  • ERR: 21
  • EDD: 4
  • MTR Big Diffs: 3
  • ESO Big Diffs: 9
  • SSZ Small Diffs: 16
  • DELightIn: 1
  • DELightOut: 1
  • JSON Small Diffs: 1

Copy link

⚠️ Regressions detected on macos-14 for commit 7ce4cfd

Regression Summary
  • ESO Small Diffs: 650
  • MTR Small Diffs: 539
  • EIO: 555
  • Table Small Diffs: 442
  • Table String Diffs: 212
  • ZSZ Small Diffs: 50
  • JSON Big Diffs: 3
  • Table Big Diffs: 27
  • ERR: 21
  • EDD: 4
  • MTR Big Diffs: 3
  • ESO Big Diffs: 9
  • SSZ Small Diffs: 16
  • DELightIn: 1
  • DELightOut: 1
  • JSON Small Diffs: 1

@amirroth
Copy link
Collaborator Author

⚠️ Regressions detected on macos-14 for commit 7ce4cfd

Regression Summary

I can't reproduce these diffs locally, in either debug or release. Did something go wrong with how this branch is attached to the repository? I did change the computer I was using to work on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DoNotPublish Includes changes that shouldn't be reported in the changelog Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants