New Coil Sizing Reports #6454

mjwitte · 2018-01-31T18:22:56Z

Pull request overview

Add two new table output reports:
Coil Sizing Details Report - Detailed coil sizing data table contributed by Trane. Report key is "CoilSizingDetails"
Coil Sizing Summary subtable in the HVAC Sizing SummaryReport. This is a subset of the Coil Sizing Details Report.

For complete code and documentation changes, see also #6500,

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.

Contribution from TRANE for comprehensive coil information table.

add description of table columns to Output Details and Examples documentation, Trane Contribution

…t-Table

…Report # Conflicts: # tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.cc

nrel-bot-3 · 2018-02-16T01:14:11Z

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

Myoldmopar · 2018-02-16T21:47:21Z

Custom check is reporting that the license year is bad in the ReportCoilSelection.* files.

The signed integer warning is because vector.size() returns an "unsigned" integer value, but you declared your loop variable as an integer. Although this won't cause any problems for this particular case, the compiler still warns about comparing a signed versus unsigned integer. You can either make your loop variable a size_t, instead of a plain integer, or you can cast the result of the size() call to a regular int. Let me know if that is super muddy.

The reorder warning is because coilCapFTIdealPeak( 1.0 ) is defined before coilRatedTotCap in the class definition, but initialized after it in the list of constructor initializations. This won't cause a problem for a simple data type like this, but the compiler still warns. Just make sure the order in the constructor initialization list matches the order in the class and this will be good to go.

I'm building locally to find the problem with the integration tests.

mjwitte · 2018-02-16T22:54:23Z

@Myoldmopar Thanks. I think I've addressed all of those. I'll wait to push until I hear if you've found anything on the watercoil test failures.

Myoldmopar · 2018-02-17T13:18:44Z

As you expected, DesiccantDehumidifierWithAirToAirCoil.idf encounters a divide by zero on mass flow rate:

Program received signal SIGFPE, Arithmetic exception.
0x00007ffff4c2ed82 in EnergyPlus::DXCoils::CalcTotCapSHR (InletDryBulb=26.666699999999999, InletHumRat=0.011178906890772281, 
    InletEnthalpy=55307.703692498355, InletWetBulb=19.428230207360055, AirMassFlowRatio=1, AirMassFlow=0, TotCapNom=14286, CBF=0, 
    CCapFTemp=3, CCapFFlow=9, TotCap=@0x7fffffffa368: 4.635571359542523e-310, SHR=@0x7fffffffa370: 6.9533558066990486e-310, 
    CondInletTemp=35, Pressure=101217, TotCapModFac=@0x55555652d280: 0.99986245570042243)
    at /src/EnergyPlus/DXCoils.cc:10211
10211				hDelta = TotCapCalc / AirMassFlow;

Myoldmopar · 2018-02-17T13:20:09Z

Tested two more of the failures, same divide by zero on that line.

…it tests

Myoldmopar · 2018-02-18T02:59:10Z

@mjwitte OK, with commit 41ab0aa, I believe I have all the unit test segfaults fixed and the integration failures addressed. But you'll want to look close at that commit to make sure my changes make sense.

In the cc file, there was a divide by zero that I mentioned earlier, and I tried to protect against that by leaving that function early. However, I'm not sure if my TotCap and SHR set to zero fix is complete enough, or even correct. So please verify that.
Second, the code was segfaulting because the coilSelectionReportObj variable was being used uninitialized. I inspected the code and found it really just needed a call to the createCoilSelectionReportObj() function. Adding that to each of the unit tests before each call to SizeWaterCoil made the unit tests pass successfully, so that seems OK.

Maybe that's all that's needed here. CI will tell us. And you can tell me if my changes make sense to you as well.

Myoldmopar · 2018-02-18T03:56:44Z

OK, this is looking way better. @mjwitte please confirm my changes as per the comment above, and make any changes needed on there. Once you then push up your changes to clean up the build warnings, this should be ready to go in.

Myoldmopar · 2018-02-18T15:06:10Z

Thanks for pushing that @EnergyArchmage. I was hesitant to do it because @mjwitte mentioned he had addressed them locally but just hadn't pushed yet, so I didn't want to stomp on his work. But he ran out of time didn't he! 😆

mjwitte

Thanks @Myoldmopar and @EnergyArchmage for the cleanups. This still needs the IDD changes for the two new report names. Plus it needs the code to produce the smaller version of this report. Soon.

mjwitte · 2018-02-18T18:54:15Z

tst/EnergyPlus/unit/WaterCoils.unit.cc

@@ -216,6 +217,7 @@ TEST_F( WaterCoilsTest, WaterCoolingCoilSizing )
 	DataWaterLoopNum = 1;
 	NumOfGlycols = 1;

+	createCoilSelectionReportObj();


@Myoldmopar Thanks. I had added this to energyplusfixture but I forgot that these tests use their own fixture.

mjwitte · 2018-02-18T18:59:50Z

src/EnergyPlus/DXCoils.cc

+		if ( AirMassFlow <= 0.00001 ) {
+			TotCap = 0.0;
+			SHR = 0.0;
+			return;


@Myoldmopar Thanks. This looks ok. Wondering why we're getting here with zero airflow, but that can be a followup investigation.

EnergyArchmage · 2018-02-18T22:56:53Z

It could be that the cooling capacity changes are causing diffs in three files are because of some subtle bug fixes that got in here. The calls to RequestSizing need to set the temporary working variable to AutoSize prior to the call -- but this isn't always being done properly. If the value passed in is not autosize value then the central sizing methods don't execute as expected. This set of code changes includes this subtle fix in WaterCoils.cc in the following locations.

https://github.com/NREL/EnergyPlus/blob/CoilSizingReport/src/EnergyPlus/WaterCoils.cc#L1928
https://github.com/NREL/EnergyPlus/blob/CoilSizingReport/src/EnergyPlus/WaterCoils.cc#L1944
https://github.com/NREL/EnergyPlus/blob/CoilSizingReport/src/EnergyPlus/WaterCoils.cc#L2208
https://github.com/NREL/EnergyPlus/blob/CoilSizingReport/src/EnergyPlus/WaterCoils.cc#L2221

@rraustad might be able to confirm, but I would guess that the diffs in 3 regression files are because of real changes in cooling coil capacity that are caused by this correction. (not otherwise document in a CR I think)

coil sizing report store design vol flow build warngin.

mjwitte · 2018-02-18T23:06:09Z

@EnergyArchmage We should pull those changes out of here and make that a separate PR. I can do that along with the other changes I need to do.

mjwitte · 2018-02-19T00:18:47Z

Currently this code adds the full detailed coil selection data as a "Coil Sizing Summary Report" subtable in "Report: HVAC Sizing Summary" (HVACSizingSummary). I think this is a good spot for an abbreviated coil sizing table with about a dozen columns. Then I propose adding a new "CoilSizingDetails" report that produces the full coil selection table as a separate report. The detailed report would be included as part of "AllSummary" reports.

@EnergyArchmage @rraustad @Myoldmopar Sound OK?

An example of the current full report inside HVAC Sizing Summary is in the attached zip file.
5ZoneAirCooledConvCoefTable.zip

EnergyArchmage · 2018-02-19T01:30:15Z

@mjwitte yes that sounds good. I originally intended to do as a separate report request but the extra effort seemed considerable compared to adding under HVACSizingSummary.

mjwitte · 2018-02-19T02:25:56Z

@EnergyArchmage For standard format reports it's not too bad. newPreDefReport automates most of it. This branch now has the new CoilSizingDetails report (in full glory) and an abbreviated Coil Sizing Summary Report subtable in the HVAC Sizing Summary. There's probably too many columns yet in the smaller version.

mjwitte · 2018-02-19T02:33:46Z

Forgot to revert the water coil diff-causers. There now.

rraustad · 2018-02-20T14:02:44Z

src/EnergyPlus/HVACDXSystem.cc

@@ -619,7 +623,11 @@ namespace HVACDXSystem {
 			// considered as as 100% DOAS DX cooling coil
 			if ( DXCoolingSystem( DXCoolSysNum ).ISHundredPercentDOASDXCoil ) {
 				// set the system DX Coil application type to the child DX coil
-				SetDXCoilTypeData( DXCoolingSystem( DXCoolSysNum ).CoolingCoilName );
+				if ( ! ( DXCoolingSystem( DXCoolSysNum ).CoolingCoilType_Num == Coil_CoolingAirToAirVariableSpeed ) ) {


interesting syntax

rraustad · 2018-02-20T14:16:51Z

src/EnergyPlus/WaterCoils.cc

@@ -1783,6 +1878,8 @@ namespace WaterCoils {
 				SizingString = WaterCoilNumericFields( CoilNum ).FieldNames( FieldNum ) + " [C]";
 				DataFlowUsedForSizing = DataAirFlowUsedForSizing; // used by air loop coils
 				TempSize = WaterCoil( CoilNum ).DesInletAirTemp;
+				TempSize = AutoSize;


This erases the user input for design inlet air temp. 2 files shows diff's in eio because of this.

5ZoneAirCooled_ZoneAirMassFlowBalance:

Component Sizing Information, Coil:Cooling:Water, MAIN COOLING COIL 1, Design Size Design Inlet Air Temperature [C], 21.00753 - Component Sizing Information, Coil:Cooling:Water, MAIN COOLING COIL 1, User-Specified Design Inlet Air Temperature [C], 21.60000 Component Sizing Information, Coil:Cooling:Water, MAIN COOLING COIL 1, Design Size Design Inlet Water Temperature [C], 7.00000 - Component Sizing Information, Coil:Cooling:Water, MAIN COOLING COIL 1, User-Specified Design Inlet Water Temperature [C], 7.00000 Coil:Cooling:Water, Main Cooling Coil 1, !- Name CoolingCoilAvailSched, !- Availability Schedule Name autosize, !- Design Water Flow Rate {m3/s} autosize, !- Design Air Flow Rate {m3/s} 7.0, !- Design Inlet Water Temperature {C} 21.6, !- Design Inlet Air Temperature {C}

I suggest a followup issue to correct any last minute findings.

rraustad · 2018-02-20T14:50:49Z

Wow! Lot's of new information now. There are some things we could probably improve, like identifying zone name for TU heating coils or finding the sensible capacity and UA for water cooling coils that don't currently have that available (Coil Sizing Details). This should merge and we can iron out the issues.

mjwitte · 2018-02-20T14:52:00Z

@rraustad There's 3 files with big eso diffs - maybe you can track those down?

rraustad · 2018-02-20T14:57:34Z

I think I just did. Sizing is ignoring the user inputs for the cooling water coil and resulting in a different size. All 3 have different cooling coil size. At least that is where I would start looking.

How do you guys want to handle these last IDD change PR's. Split them up and work towards finalizing? Or just merge ones that are likely and follow up issue those that need some correcting after IDD freeze?

mjwitte · 2018-02-20T16:19:46Z

@rraustad If you have a fix for this, please point me to it, and I'll include that along with a section in output rules and a merge up to current develop. Trying to minimize pushes.

rraustad · 2018-02-20T17:14:41Z

WaterCoils.cc line 1881. bPRINT = true here if coil type is water simple. So using TempSize = AutoSize will neglect the user input for design inlet water temp. Why was this line added? I assume to be able to report sizing at actual vs design conditions.

rraustad · 2018-02-20T17:22:26Z

and line 1897. I see where you reverted these out but must have missed these. Why do this anyway?

EnergyArchmage · 2018-02-20T18:40:57Z

I had added several of those TempSize = AutoSize statements to get more of the code to execute inside the sizing routine. I don't recall the details.

…Report Conflicts: src/EnergyPlus/DataSizing.hh src/EnergyPlus/HVACUnitarySystem.cc

mjwitte · 2018-02-20T20:27:10Z

@Myoldmopar This is ready for merge (with followups noted in #6481) once CI reports back, if you agree.

mjwitte · 2018-02-20T20:38:39Z

@Myoldmopar Note that we expect a few diffs, similar to a few days ago.
619 tests had: AUD diffs.
3 tests had: EIO diffs.
3 tests had: ESO big diffs.
3 tests had: Table big diffs.
101 tests had: ERR diffs.
1 test had: Table small diffs.
1 test had: MTR big diffs.
Failures:

regression Test Summary

Passed: 616
Failed: 3

The err warnings are in a lot of files - sorry.

mjwitte · 2018-02-20T21:09:16Z

The units warnings should be gone now. Sorry to kick off another cycle.

Myoldmopar · 2018-02-20T22:09:03Z

@mjwitte this is going in. We have to get the JSON finalized, and this needs to go first. If you still see a problem with these results, please add it to the follow-up.

EnergyArchmage and others added 2 commits January 25, 2018 12:13

draft code changes for nwe coil summary report table

f6453b1

Contribution from TRANE for comprehensive coil information table.

CoilSizingReport-NFP

6e2d192

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 Jan 31, 2018

mjwitte added this to the EnergyPlus 8.9.0 milestone Jan 31, 2018

EnergyArchmage and others added 8 commits February 5, 2018 10:20

Doc changes for coil report table

dcfcab4

add description of table columns to Output Details and Examples documentation, Trane Contribution

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

d6c7e31

…t-Table

Coil sizing report - fix test failures

d395b27

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

aa64ea0

…t-Table

Coil sizing report - protect against errors for fan not found

1ee7c8d

Coil sizing report - fix test failures 2 and build warnings

1c22fd3

Merge branch 'Coil-Report-Table' into CoilSizingReport

156fac0

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

89809a9

…Report # Conflicts: # tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.cc

Myoldmopar added 2 commits February 17, 2018 20:53

Add test for zero mass flow, fix missing pointer initialization in un…

41ab0aa

…it tests

Merge remote-tracking branch 'origin/develop' into CoilSizingReport

ed568c1

fix build warnings in coil sizing report

314cdd5

mjwitte commented Feb 18, 2018

View reviewed changes

fix uninitialized variable build warning

5868641

coil sizing report store design vol flow build warngin.

Coil sizing report - Revert water coil fixes causing diffs

57a0055

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

rraustad reviewed Feb 20, 2018

View reviewed changes

mjwitte mentioned this pull request Feb 20, 2018

Additional Terminal Unit Sizing Issues #6481

Closed

11 tasks

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

mjwitte added 3 commits February 20, 2018 13:43

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

515082b

…Report Conflicts: src/EnergyPlus/DataSizing.hh src/EnergyPlus/HVACUnitarySystem.cc

Coil sizing report - fix merge mistake

b20c099

Coil sizing report - minor title change and add output rules

6bb8d01

mjwitte mentioned this pull request Feb 20, 2018

JSON input format and InputProcessor refactor #5893

Merged

20 tasks

Coil sizing report - fix units warnings

bb00e38

Myoldmopar merged commit e592307 into develop Feb 20, 2018

Myoldmopar deleted the CoilSizingReport branch February 20, 2018 22:09

mjwitte mentioned this pull request Feb 26, 2018

Change remaining Coil:Heating:Gas to Fuel plus cleanups on new coil sizing reports #6500

Merged

29 tasks

mjwitte mentioned this pull request Aug 8, 2018

Stray test strings showing in EquipmentSummary Fans table for fan sizing peak day and time #6899

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Coil Sizing Reports #6454

New Coil Sizing Reports #6454

mjwitte commented Jan 31, 2018 •

edited

Loading

nrel-bot-3 commented Feb 16, 2018

Myoldmopar commented Feb 16, 2018

mjwitte commented Feb 16, 2018

Myoldmopar commented Feb 17, 2018

Myoldmopar commented Feb 17, 2018

Myoldmopar commented Feb 18, 2018

Myoldmopar commented Feb 18, 2018

Myoldmopar commented Feb 18, 2018

mjwitte left a comment

mjwitte Feb 18, 2018

mjwitte Feb 18, 2018

EnergyArchmage commented Feb 18, 2018

mjwitte commented Feb 18, 2018

mjwitte commented Feb 19, 2018

EnergyArchmage commented Feb 19, 2018

mjwitte commented Feb 19, 2018

mjwitte commented Feb 19, 2018

rraustad Feb 20, 2018

rraustad Feb 20, 2018

rraustad commented Feb 20, 2018

mjwitte commented Feb 20, 2018

rraustad commented Feb 20, 2018

mjwitte commented Feb 20, 2018

rraustad commented Feb 20, 2018

rraustad commented Feb 20, 2018

EnergyArchmage commented Feb 20, 2018

mjwitte commented Feb 20, 2018

mjwitte commented Feb 20, 2018

mjwitte commented Feb 20, 2018

Myoldmopar commented Feb 20, 2018

New Coil Sizing Reports #6454

New Coil Sizing Reports #6454

Conversation

mjwitte commented Jan 31, 2018 • edited Loading

Pull request overview

Work Checklist

Review Checklist

nrel-bot-3 commented Feb 16, 2018

Myoldmopar commented Feb 16, 2018

mjwitte commented Feb 16, 2018

Myoldmopar commented Feb 17, 2018

Myoldmopar commented Feb 17, 2018

Myoldmopar commented Feb 18, 2018

Myoldmopar commented Feb 18, 2018

Myoldmopar commented Feb 18, 2018

mjwitte left a comment

Choose a reason for hiding this comment

mjwitte Feb 18, 2018

Choose a reason for hiding this comment

mjwitte Feb 18, 2018

Choose a reason for hiding this comment

EnergyArchmage commented Feb 18, 2018

mjwitte commented Feb 18, 2018

mjwitte commented Feb 19, 2018

EnergyArchmage commented Feb 19, 2018

mjwitte commented Feb 19, 2018

mjwitte commented Feb 19, 2018

rraustad Feb 20, 2018

Choose a reason for hiding this comment

rraustad Feb 20, 2018

Choose a reason for hiding this comment

rraustad commented Feb 20, 2018

mjwitte commented Feb 20, 2018

rraustad commented Feb 20, 2018

mjwitte commented Feb 20, 2018

rraustad commented Feb 20, 2018

rraustad commented Feb 20, 2018

EnergyArchmage commented Feb 20, 2018

mjwitte commented Feb 20, 2018

mjwitte commented Feb 20, 2018

mjwitte commented Feb 20, 2018

Myoldmopar commented Feb 20, 2018

mjwitte commented Jan 31, 2018 •

edited

Loading