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

Glazing enhancements - input glazing properties as a function of incident angle #6114

Merged
merged 18 commits into from
Jul 7, 2017

Conversation

lgu1234
Copy link
Contributor

@lgu1234 lgu1234 commented May 2, 2017

Pull request overview

This new feature allows users to input glazing properties of transmittance, front reflectance, and back reflectance as a function of incident angle, in addition to existing inputs as a function of wavelength.

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.

  • Code style (parentheses padding, variable names)
  • Functional code review (it has to work!)
  • n/a If defect, results of running current develop vs this branch should exhibit the fix
  • CI status: all green or justified
  • Performance: CI Linux results include performance check -- verify this
  • Unit Test(s)
  • C++ checks:
    • Argument types
    • If any virtual classes, ensure virtual destructor included, other things
  • IDD changes:
    • Verify naming conventions and styles, memos and notes and defaults
    • Open windows IDF Editor with modified IDD to check for errors - posted IDF Editor cleanup for v8.8 release #6199
    • If transition, add rules to spreadsheet
    • If transition, add transition source
    • If transition, update idfs
  • If new idf included, locally check the err file and other outputs
  • Documentation changes in place
  • Changed docs build successfully
  • n/a ExpandObjects changes?
  • n/a If output changes, including tabular output structure, add to output rules file for interfaces

@mjwitte mjwitte added IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) NewFeature Includes code to add a new feature to EnergyPlus labels May 10, 2017
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.

Some initial comments on the code. Have not reviewed docs or tried the run this yet.

@@ -85322,7 +85330,122 @@ Table:TwoIndependentVariables,
N720,N721,N722,N723,N724,N725,N726,N727,N728,N729,N730,N731,N732,N733,N734,N735,N736,N737,N738,N739, \note fields as indicated
N740,N741,N742,N743,N744,N745,N746,N747,N748,N749,N750,N751,N752,N753,N754,N755,N756,N757,N758,N759, \note fields as indicated
N760,N761,N762,N763,N764,N765,N766,N767,N768,N769,N770,N771,N772,N773,N774,N775,N776,N777,N778,N779, \note fields as indicated
N780,N781,N782,N783,N784,N785,N786,N787,N788,N789,N790,N791,N792,N793,N794,N795,N796,N797,N798,N799; \note fields as indicated
N780,N781,N782,N783,N784,N785,N786,N787,N788,N789,N790,N791,N792,N793,N794,N795,N796,N797,N798,N799, \note fields as indicated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why extend this object in the idd if we expect to read the data from a file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It provides flexibility to allow optical data inputs either from a file or directly from the same idf. It is also easy to add an example without adding extra files.
If you think no needs to input data from an idf file, I can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The new example file only goes to N565 and the object is extensible, so the only reason to extend this in the IDD would be to edit a longer table object in IDF Editor. My guess is that most users will prefer the external file format, so I would drop the extra fields here and leave it at N799.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I am going to revise idd to make max N = N799,

@@ -2547,6 +2554,18 @@ namespace CurveManager {
}
}

IndVarSwitch = false;
if ( SameString( Alphas( 4 ), "WAVELENGTH" ) && SameString( Alphas( 5 ), "ANGLE" ) ) {
IndVarSwitch = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really worth all the trouble to flip the variables? The unit type fields are not required, so you can't be sure these will be input. And, in general, when a component model uses a curve/table object, it expects X1 to be a specific variable (e.g. Outdoor dry bulb temperature). Seems it would be better to enforce X1 is always angle and X2 is always wavelength (or the opposite) and in the function that calls this, check the max/min values for X1 and X2 and throw a warning if they don't line up with the proper variable type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, I can force variable types to specify X1 as angle and X2 as wavelength.

Tables( 2 ) = PerfCurve( FRefleCurveIndex ).TableIndex;
Tables( 3 ) = PerfCurve( BRefleCurveIndex ).TableIndex;

// Set up independent variable size to cover all independent varaible values in 3 tables
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo - varaible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I am going to revise them.

Actually, I found many typos in other modules. I will also fix them. See a screen shot below.

capture

int const ConstrNum,
int const NGlass,
int & TotalIPhi,
Array1A_int Tables
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent is one too many here and a few lines above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

GetCurveInterpolationMethodNum( int const CurveIndex ); // index of curve in curve array

void
ReadTableDataFromFile(
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an existing function named ReadTableData that is used by Table:MultiVariableLookup and this new function is named ReadTableDataFromFile. Since this one is specific to Table:TwoIndependentVariables, it should be renamed. Maybe ReadTwoVarTableData or ReadTwoVarTableDataFromFile .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will use ReadTwoVarTableDataFromFile.

@nrel-bot
Copy link

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

@nrel-bot-2
Copy link

@lgu1234 @lgentile it has been 15 days since this pull request was last updated.

@mjwitte
Copy link
Contributor

mjwitte commented Jun 12, 2017

@lgu1234 There are conflicts now on this branch. Also, do you want me to add the transition rule or will you? I will review soon. Also, please add an overview in the pull request describing the changes.

@lgu1234
Copy link
Contributor Author

lgu1234 commented Jun 12, 2017

@mjwitte I am going to add transition. I will name it as Rules8-7-0-to-8-8-0.md.

Conflicts:
	src/EnergyPlus/CurveManager.cc
	src/EnergyPlus/CurveManager.hh
	tst/EnergyPlus/unit/WindowManager.unit.cc
@lgu1234
Copy link
Contributor Author

lgu1234 commented Jun 12, 2017

@mjwitte Conflicts have been solved.

@mjwitte mjwitte changed the title Glazing enhancements based on Winkelmann approach Glazing enhancements - input glazing properties as a function of incident angle Jun 14, 2017
@NREL NREL deleted a comment from amirroth Jun 14, 2017
@@ -204,6 +206,10 @@ \subsubsection{Inputs}\label{inputs-1-029}
Distance
\item
Power
\item
Wavelength
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjwitte Yes. Thanks.

\item
Wavelength
\item
Angle
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjwitte Yes. Remove the Angle item.

\paragraph{Field: Window Glass Spectral and Incident Angle Transmittance Data Set Table Name}\label{field-window-glass-spectral-and-incident-angle-transmittance-data}

If Optical Data Type = SpectralAndAngle, this is the name of a spectral and angle data set of transmittance defined with a Table:TwoIndependentVariables object.

Copy link
Contributor

@mjwitte mjwitte Jun 14, 2017

Choose a reason for hiding this comment

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

Explain here (or above) that the x variable must be angle and the y variable must be wavelength, and what the format of the table file should be. Or explain that in the Table:TwoIndependetVariables object and refer to that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjwitte I am going to add explanation.

ShowContinueError( "In order to input correct variable type for optical properties, " + cAlphaFieldNames( 4 ) + " should be ANGLE, and " + cAlphaFieldNames( 5 ) + " should be WAVELENGTH " );
ErrorsFound = true;
}
if ( SameString( Alphas( 4 ), "ANGLE" ) && !SameString( Alphas( 5 ), "WAVELENGTH" ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, what happens if these fields are left blank? Does the window model check this elsewhere? Or does it check the range of values to throw a warning if they don't make sense?

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.

@lgu1234 Comments based on code review only. Haven't tested this yet. @Myoldmopar Would be good for you to review code also.

// FUNCTION INFORMATION:
// AUTHOR L. Gu
// DATE WRITTEN Feb. 2017
// MODIFIED na
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// Return value
int TableInterpolationMethodNum;

// Locals
Copy link
Contributor

Choose a reason for hiding this comment

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

more unused comments to remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


CheckForActualFileName( FileName, FileExists, TempFullFileName );
if ( !FileExists ) {
ShowSevereError( "CurveManager: SearchTableDataFile: Could not open Table Data File, expecting it as file name = " + FileName );
Copy link
Contributor

Choose a reason for hiding this comment

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

CurveManager: SearchTableDatafile --> CurveManager::ReadTwoVarTableDataFromFile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjwitte Done. Thanks.

ShowFatalError( "Program terminates due to these conditions." );
}

TableNum = PerfCurve( CurveNum ).TableIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a check to make sure this is a Table:TwoIndependentVariables curve type?


// Re-organize performance curve table data structure
for ( TableNum = 1; TableNum <= int( Tables.size( ) ); TableNum++ ) {
PerfCurveTableData( Tables( TableNum ) ).X1.allocate( int( XX1.size( ) ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it proper to allocate again (here and below) without first deallocating these? Or is redimension better here? @Myoldmopar

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't redimension. If the Arrays function like std::vector, which I believe they do, then you can just allocate again and it will work properly.

if ( !BGFlag ) AllGlassIsSpectralAverage = false;
numptDAT = TableLookup( PerfCurve( Material( LayPtr ).GlassSpecAngTransDataPtr ).TableIndex ).NumX2Vars;
numpt( IGlass ) = numptDAT;
TotalIPhi = TableLookup( PerfCurve( Material( LayPtr ).GlassSpecAngTransDataPtr ).TableIndex ).NumX1Vars;
Copy link
Contributor

Choose a reason for hiding this comment

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

MaxNumOfIncidentAngles is hardcoded to 20, what if this comes back with TotalIPhi > 20? Has that been checked elsewhere? Or should be checked earlier and MaxNumOfIncidentAngles set to the greatest of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has not been checked. I am going to add a check.

Array1D< Real64 > CosPhiIndepVar( MaxNumOfIncidentAngles, 0.0 ); // Cos of incidence angles at 10-deg increments for curve fits

Array1D< int > LayerNum( 5, 0 ); // Glass layer number
Array1D< int > AngleNum( 5, 0 ); // Glass layer number for spetral and angular data only
Copy link
Contributor

Choose a reason for hiding this comment

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

spetral --> spectral

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

AngleNum( IGlass ) = LayerNum( IGlass );
}
}
SetCommonIncidentAngles( ConstrNum, NGlass, TotalIPhi, AngleNum );
Copy link
Contributor

Choose a reason for hiding this comment

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

Having trouble following all of the logic here, but it seems like SetCommonIncidentAngles should be inside an if that checks if this construction even includes a spectralandangle layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjwitte Actually, this check needs to be performed as long as a single layer is SpectralAndAngle. For example, one layer is Spectral and other layer is SpectralAndAngle. If none of layers is SpectralAndAngle, the function will return without setting common incident angle. This is checked inside the function.

Phi = double( IPhi - 1 ) * 10.0;
for ( IGlass = 1; IGlass <= NGlass; ++IGlass ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment here would be good to explain that the normal value for Phi above gets overridden if there's a layer that is SpectralAndAngle. And that TotalIPhi is 10 when it's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments are added.

}
} // End of loop over glass layers in the construction for back calculation

// Loop over incidence angle from 0 to 90 deg in 10 deg increments.
// Get bare glass layer properties, then glazing system properties at each incidence angle.
// The glazing system properties include the effect of inter-reflection among glass layers,
// but exclude the effect of a shade or blind if present in the construction.
for ( IPhi = 1; IPhi <= 10; ++IPhi ) {
for ( IPhi = 1; IPhi <= TotalIPhi; ++IPhi ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, a comment here explaining the difference for SpectralAndAngle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments are added.

}

// ascend sort
for ( i = 1; i <= int( XX1.size() ); i++ ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use std::sort() instead. It will be faster O(n log n) vs O(n^2) and it is less code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbadams5 Do we allow to use std::sort? In addition, how to make descend or ascend? Could you provide an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you can and should use std::sort().
The documentation for sort is here.

This is untested code, but it should be as follows.

std::sort( XX1.begin(), XX1.end() );

By default, it will sort in ascending order. If you want to sort in descending order you can do the following...

std::sort( XX1.begin(), XX1.end(), []( Real64 i, Real64 j ){ return i > j; } );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbadams5 Changed.

}
}

for ( i = 1; i <= int( XX2.size( ) ); i++ ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use std::sort() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbadams5 Changed. Thanks.

if ( !Anglefound ) return;

// ascend sort
for ( i = 1; i <= int( XX1.size( ) ); i++ ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use std::sort() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


// Re-organize performance curve table data structure
for ( TableNum = 1; TableNum <= int( Tables.size( ) ); TableNum++ ) {
PerfCurveTableData( Tables( TableNum ) ).X1.allocate( int( XX1.size( ) ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't redimension. If the Arrays function like std::vector, which I believe they do, then you can just allocate again and it will work properly.

@lgu1234
Copy link
Contributor Author

lgu1234 commented Jun 14, 2017

@mbadams5 You just said: Don't redimension and just allocate again with new size. Can the array keep old values before new allocate?

@mbadams5
Copy link
Contributor

@lgu1234 I don't really understand what you mean by "Can the array keep old values before new allocate". If you call allocate on the array, it will (or should) allocate a new array and all the old data in the array will be gone.

@lgu1234
Copy link
Contributor Author

lgu1234 commented Jun 15, 2017

@mbadams5 I am not clear what you request. The following line is allocate for X1 for a new size:

PerfCurveTableData( Tables( TableNum ) ).X1.allocate( int( XX1.size( ) ) );

Could you let me know which way is better?

@mbadams5
Copy link
Contributor

@lgu1234 I was responding to a comment made by @mjwitte. The way it is currently coded is correct and you should leave it.

Conflicts:
	src/EnergyPlus/DataHeatBalance.hh
	src/Transition/InputRulesFiles/Rules8-7-0-to-8-8-0.md
@@ -1303,6 +1305,19 @@ \subsubsection{Inputs}\label{inputs-13-015}

The ratio, when a sample object is stretched, of the contraction or transverse strain (perpendicular to the applied load), to the extension or axial strain (in the direction of the applied load). This value is used only with complex fenestration systems defined through the Construction:ComplexFenestrationState object. The default value for glass is 0.22.

\paragraph{Field: Window Glass Spectral and Incident Angle Transmittance Data Set Table Name}\label{field-window-glass-spectral-and-incident-angle-transmittance-data}

If Optical Data Type = SpectralAndAngle, this is the name of a spectral and angle data set of transmittance defined with a Table:TwoIndependentVariables object. The first and second independent variables must be Angle, and Wavelenght, respectively. The restriction is based on iternal dataset use. Each dateset is divided into subsets for each inceident angle internally.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below:
Wavelenght --> Wavelength
dateset --> dataset
inceident --> incident

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

\type real
N19, \field Output Value #4
\required-field
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that these are not required fields, will the getinput throw an error if these fields are not present when used without an external file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -292,6 +293,7 @@ namespace CurveManager {
Array1D< TriQuadraticCurveDataStruct > Tri2ndOrder; // structure for triquadratic curve data
bool EMSOverrideOn; // if TRUE, then EMS is calling to override curve value
Real64 EMSOverrideCurveValue; // Value of curve result EMS is directing to use
bool OpticalProperty; // if TRUE, this table is used to stro optical property
Copy link
Contributor

Choose a reason for hiding this comment

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

stro --> store

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

int const ConstrNum,
int const NGlass,
int & TotalIPhi,
Array1A_int Tables
Copy link
Contributor

Choose a reason for hiding this comment

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

It's hard to follow what this array represents here. From the calling function, I think it's an array of size NGlass (which is the number of glass layers in construction ConstrNum) which holds the material layer numbers for the glass layers which are SpectralAndAngle. For glass layers which are not SpectralAndAngle, this will be zero. This at least needs a comment to define what this array is. A different variable name might make this function more readable. This is too wordy, but my understanding of this array is SpectralAndAngleGlassLayerMaterialLayerPointers ? Or TableMaterialPointers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are comments in CurveManger.hh
int const ConstrNum, // Construction number
int const NGlass, // The number of glass layers in the construction with index = ConstrNum
int & TotalIPhi, // The number of incident angles
Array1A_int const Tables // Glass layer number for SpectralAndAngleGlassLayer only. Otherwise = 0 for other layers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Much better, but the value in Tables is the Glass layer material number, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is glass layer number with optical property of SpectralAndAngle. Material number is Construct( ConstrNum ).LayerPoint( AngleNum( IGlass ) ).

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, OK, but "Glass layer number" is still confusing, because of the dual layer numbering, one just for glass layers, and the other for all layers in the construction. This one is within all material layers in the construction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. It should be construction layer number for SpectralAndAngle glass. Hope it is clear enough.

! SpectralAngularOpticalProperties_TableData.idf
! Basic file description: 1 story building divided into 3 interior conditioned zones and 1 unconditioned zone over the
! conditioned zones. Roof with no plenum. No ground contact with floor.
! Outdoor air is modulated based on occupancy and floor area.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this based on another example file? That would be useful to mention here.

!
! Highlights: This file does the basic test of window properties as a function of wavelength and incident angle
! based on Winklemann approach. Table adta are uased to input optical properties. Linear interpolation
! is porformed to caluclate optical properties with a given incident angle.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some typos in this paragraph. "adta", "uased", "porformed".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

std::ifstream infile( FileName );
std::string line;

CheckForActualFileName( FileName, FileExists, TempFullFileName );
Copy link
Contributor

Choose a reason for hiding this comment

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

@lgu1234 I found the problem with reading the file on my machine. When run with ep-launch, CheckForActualFileName looks in several places to find the file and sets TempFullFileName. But here, infile is already set using FileName. So, when I run this, FileExists is true, but later on the std::getline( infile, line ) comes back false (wrong path) and he while loop never executes and lineNum is zero.

ShowFatalError( "Program terminates due to these conditions." );
}

TableNum = PerfCurve( CurveNum ).TableIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you move std::ifstream infile( FileName ); here from above and use TempFullFileName it solves the problem on my machine running with EP-Launch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for find the problem. Here is my understanding:

  1. Remove the line " std::ifstream infile( FileName );"
  2. Replace "infile" by TempFullFileName in the line below
    while (std::getline( infile, line )) {
    Please let me know if these actions are OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you need to insert the std::ifstream... line here using TempFullFileName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding was wrong. Here are my changes:

	TableNum = PerfCurve( CurveNum ).TableIndex;
	std::ifstream infile( TempFullFileName );

Please confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's good.

TableData( TableNum ).Y.allocate( 0 );
lineNum = 0;
while (std::getline( infile, line )) {
std::vector<std::string> strings = splitString( line, ',' );
Copy link
Contributor

Choose a reason for hiding this comment

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

@Myoldmopar is this method ok here? All other file reads are using gio functions, such as ReadTableData. This is different here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is from OpenStudio and recommended by Jason.

TableData( TableNum ).Y.push_back( std::stod( strings[2] ) );
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add a fatal error here if lineNum is zero for whatever reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

External file fix works now. I/O Ref looks good. Question below about Engineering Ref (and typo). Added and tested the transition rule and fixed some typos in the example file. Waiting for CI results to come back. @Myoldmopar Do you want to look at this again before merge?

@@ -392,6 +392,8 @@ \subsection{Glazing System Properties}\label{glazing-system-properties}

where \({E_s}(\lambda )\) is the solar spectral irradiance function and \(V(\lambda )\) is the photopic response function of the eye. The default functions are shown in Table~\ref{table:solar-spectral-irradiance-function.} and Table~\ref{table:photopic-response-function.}. They can be overwritten by user defined solar and/or visible spectrum using the objects Site:SolarAndVisibleSpectrum and Site:SpectrumData. They are expressed as a set of values followed by the corresponding wavelengths for values.

When a choice of Spectral is entered as optical data type, the correlations to store glazing system's angular performance are generated based on angular performance at 10 degree increments. When a choice of SpectralAndAngel is entered as optical data type, no correlations will be generated. The optical properties will be calculated by linear interpolation at a given incident angle.
Copy link
Contributor

Choose a reason for hiding this comment

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

@lgu1234 Does this need to be revised? Also, SpectralAndAngel-->SpectralAndAngle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjwitte Here is my revision.

When a choice of Spectral is entered as optical data type, the correlations to store glazing system's angular performance are generated based on angular performance at 10 degree increments. When a choice of SpectralAndAngle is entered as optical data type, no correlations will be generated. The optical properties will be calculated by linear interpolation at a given incident angle to generate table data. The common incident angles are applied to all glass layers in the same construction. These table data of optical properties are used to generate polynomial curve fits with 6 coefficients as a function of cos of incident angles. The polynomial curves are used in simulations to calculate optical properties.

Please let me know your opinion.

@mjwitte mjwitte mentioned this pull request Jul 5, 2017
4 tasks
@mjwitte
Copy link
Contributor

mjwitte commented Jul 5, 2017

Also, @Myoldmopar when I built transition locally, the v8-7-0-Energy+.idd was overwritten with the contents of the current idd instead of making a v8-8-0-Energy+.idd Is there something that needs fixing in the cmake files or is this how it is until we move the main version number forward?

@mjwitte mjwitte merged commit db38b38 into develop Jul 7, 2017
@mjwitte mjwitte deleted the GlazingEnahncementsWinkelmann branch July 7, 2017 21:17
@mjwitte mjwitte mentioned this pull request Jul 8, 2017
20 tasks
@Myoldmopar Myoldmopar mentioned this pull request Aug 21, 2017
20 tasks
@mjwitte mjwitte mentioned this pull request Jul 27, 2018
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) NewFeature Includes code to add a new feature to EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants