Change remaining Coil:Heating:Gas to Fuel plus cleanups on new coil sizing reports by mjwitte · Pull Request #6500 · NatLabRockies/EnergyPlus

mjwitte · 2018-02-26T22:09:27Z

Pull request overview

Part 2 of new coil sizing reports, completing #6454.

Fix plant index errors
Add design document
Finish documentation
Add unit tests
Refine units in column headings - use standard units enums or equivalent?
Add timestamp tag to date/time columns

Also . . .

Fixed lingering occurrences of Coil:Heating:Gas --> Fuel, which actually were discovered by one of these new unit tests (Coil:Heating:Fuel wasn't being recognized as a heating coil)
Noted ip-units conversion warnings for table outputs and fixed units in the Surface Convection Parameters eio output header.
Partially addresses Additional Terminal Unit Sizing Issues #6481

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.

Myoldmopar

This looks really good. I had a question about the unit tests. Let me know what you think there and we'll get it approved.

Myoldmopar · 2018-02-28T17:12:06Z

src/EnergyPlus/ReportCoilSelection.cc

+)
+{
+	int index = getIndexForOrCreateDataObjFromCoilName( coilName,coilType );
+	auto & c( coilSelectionDataObjs[ index ] );


Myoldmopar · 2018-02-28T17:12:58Z

src/EnergyPlus/ReportCoilSelection.cc

+	if ( c->waterLoopNum > 0 && c->pltSizNum > 0 ) {
+		c->plantLoopName = DataPlant::PlantLoop( c->waterLoopNum ).Name;
+		if ( DataSizing::PlantSizData( c->pltSizNum  ).LoopType != DataSizing::SteamLoop ) {
+			c->rhoFluid = FluidProperties::GetDensityGlycol( DataPlant::PlantLoop( c->waterLoopNum ).FluidName, DataGlobals::InitConvTemp, DataPlant::PlantLoop( c->waterLoopNum ).FluidIndex, "ReportCoilSelection::setCoilWaterFlow" );


Thanks for being explicit on the scope of these functions and not using using statements!

Myoldmopar · 2018-02-28T17:13:24Z

src/EnergyPlus/ReportCoilSelection.cc

+	if ( isAutoSized ) {
+		c->coilWaterFlowAutoMsg = "Yes" ;
+	} else {
+		c->coilWaterFlowAutoMsg = "No" ;


So easy to read all this with the c reference variable.

Myoldmopar · 2018-02-28T17:14:30Z

src/EnergyPlus/ReportCoilSelection.hh

 		Real64 const totalHeatingCap, // {W} coil Heating capacity
 		bool const isAutoSize, // true if value was autosized
-		int const dataPltSizNum, // plant sizing structure index
+		int const DataPltSizNum, // plant sizing structure index


Was there a reason for capitalizing this?

Hmm, don't remember changing that.

Myoldmopar · 2018-02-28T17:15:38Z

tst/EnergyPlus/unit/ReportCoilSelection.unit.cc

+	Real64 airVdot ( 0.052 ); // air flow rate in m3/s
+	bool isAutoSized ( false ); // true if air flow was autosized
+
+	coilSelectionReportObj->setCoilAirFlow( coilName, coilType, airVdot, isAutoSized );


So concise. Are you adding more unit tests or is this the extent of your testing?

From the coverage results it looks like the code that isn't covered by this test is mostly the erroneous code paths and the steam loop calculations. However there is a section in the setCoilWaterFlowNodeNums function where c->waterLoopNum > 0 && c->pltSizNum > 0 and that block doesn't get hit. What do you think about adding some coverage there?

This is only half-baked. Will add more. The functions themselves are all from @EnergyArchmage so I can't take credit for how they're written. I just made copies of them to make two flavors for some.

mjwitte · 2018-02-28T17:45:25Z

src/EnergyPlus/ReportCoilSelection.hh



-private: // data
+public: // data


@Myoldmopar @EnergyArchmage I couldn't get the unit tests to work without making this public. Is there a better way?

https://stackoverflow.com/questions/2396370/how-to-make-google-test-classes-friends-with-my-classes

I think make it public. Or better yet make your class a struct and just make things private that are definitely not needed anywhere else even in unit tests.

…Followup # Conflicts: # src/EnergyPlus/WaterCoils.cc

mjwitte · 2018-03-13T21:17:01Z

tst/EnergyPlus/unit/ReportCoilSelection.unit.cc

+	coilSelectionReportObj->setCoilAirFlow( coil1Name, coil1Type, airVdot, isAutoSized );
+	auto & c1 ( coilSelectionReportObj->coilSelectionDataObjs[ 0 ] );
+	EXPECT_EQ( coil1Name, c1->coilName_ );
+	EXPECT_EQ( coil1Type, c1->coilObjName );


@Myoldmopar This works here and I can query c1 and see that's filled where it should be. But later on after coil 2 is created, c1 becomes undefined. What needs to be different here when declaring c1 to make it stick later?

mjwitte · 2018-03-13T21:17:24Z

tst/EnergyPlus/unit/ReportCoilSelection.unit.cc

+	// First with no plant sizing objects defined
+	isAutoSized = false; // true if autosized
+	coilSelectionReportObj->setCoilWaterFlowNodeNums( coil1Name, coil1Type, waterVdot,  isAutoSized, chWInletNodeNum, chWOutletNodeNum, loopNum );
+	EXPECT_EQ( -999, c1->pltSizNum );


c1 is still ok here.

mjwitte · 2018-03-13T21:18:17Z

tst/EnergyPlus/unit/ReportCoilSelection.unit.cc

+	std::string coil2Type ( "Coil:Cooling:Water" ); // idf input object class name of coil
+	int pltSizNum = -999;
+	coilSelectionReportObj->setCoilWaterFlowPltSizNum( coil2Name, coil2Type, waterVdot,  isAutoSized, pltSizNum, loopNum );
+	auto & c2 ( coilSelectionReportObj->coilSelectionDataObjs[ 1 ] );


But here after a second coilSelectionDataObjs is created, c1 is undefined.

mjwitte · 2018-03-13T21:20:36Z

tst/EnergyPlus/unit/ReportCoilSelection.unit.cc

+	DataSizing::PlantSizData( 1 ).PlantLoopName = "Chilled Water Loop";
+	isAutoSized = true; // true if autosized
+	coilSelectionReportObj->setCoilWaterFlowNodeNums( coil1Name, coil1Type, waterVdot,  isAutoSized, chWInletNodeNum, chWOutletNodeNum, loopNum );
+	auto & c1b ( coilSelectionReportObj->coilSelectionDataObjs[ 0 ] );


So, here for now, creating a new c1b to point to the first coil.

change zone weighting to supply-air-flow-rate from zone volume, traps to not overwrite values set directly

…izingFollowup

…Followup

Myoldmopar · 2018-03-16T12:42:14Z

This looks really good. I am happy to see the plant index errors gone. Looking at the diffs, they seem to be only in the table, and spot checking those I see differences in coil sizes that are relatively small, but numerically significant for throwing diffs. I have no issue with this. Were you planning on doing anything else on here? If not I'll merge.

mjwitte · 2018-03-16T12:46:50Z

@Myoldmopar Just about finished with expanded unit tests and still want to clean up some units in the table headers. Should be up within the hour.

Myoldmopar · 2018-03-16T12:51:03Z

👍

…Followup

mjwitte · 2018-03-16T14:45:42Z

@Myoldmopar Note change in labels, title and new items in overview.

Myoldmopar · 2018-03-16T14:47:25Z

Thanks for covering this so well @mjwitte. I'm going to avoid merging for a little while and let CI have a few hours to check on diffs in this and a couple others that are basically ready to go. If this is clean it'll go in.

mjwitte · 2018-03-16T16:37:26Z

@Myoldmopar Arrrgh. Tablediff is failing now.
(diff, which) = hdict[h][i]
KeyError: u'Date/Time at Sensible Ideal Loads Peak'

mjwitte · 2018-03-16T16:43:31Z

src/EnergyPlus/OutputReportPredefined.cc

 		//results from regular zone and system sizing calcs, "At Ideal Loads Peak"
 		pdch2CoilDDnameSensIdealPeak = newPreDefColumn( pdst2CoilSummaryCoilSelection, "Design Day Name at Sensible Ideal Loads Peak" );
-		pdch2CoilDateTimeSensIdealPeak = newPreDefColumn( pdst2CoilSummaryCoilSelection, "Date/Time at Sensible Ideal Loads Peak" );
+		pdch2CoilDateTimeSensIdealPeak = newPreDefColumn( pdst2CoilSummaryCoilSelection, "Date/Time at Sensible Ideal Loads Peak {TIMESTAMP}" );


@JasonGlazer So, adding {TIMESTAMP} here is causing TableDiff to crash. Can you see anything wrong here?

Nothing jumps out as to why this would cause a problem.

mjwitte · 2018-03-16T19:16:07Z

tst/EnergyPlus/unit/ReportCoilSelection.unit.cc

+
+	Real64 zoneHeatingLatentLoad = 4321.0;
+	coilSelectionReportObj->setZoneLatentLoadHeatingIdealPeak( zoneNum, zoneHeatingLatentLoad );
+	// Expect zero because doZoneEqSetup isn't currently executed


@EnergyArchmage I noticed that all calls to coilSelectionReportObj->doZoneEqSetup are currently commented out. Is that your intent at this point?

Myoldmopar · 2018-03-16T20:12:58Z

Hmm....looks like it's still failing with TableDiff?

mjwitte · 2018-03-16T20:20:05Z

@JasonGlazer What the . . . can TableDiff no handle any change in a column heading?

mjwitte · 2018-03-16T20:59:38Z

One more time . . . hope TableDiff is happy now.

Myoldmopar · 2018-03-17T00:27:46Z

Back to tablediffs, not tablediff failures! Ready to go once CI continues a bit more?

Myoldmopar · 2018-03-17T02:33:53Z

Pulled in the latest develop; letting CI run overnight and we'll merge in the morning.

Myoldmopar · 2018-03-17T13:10:39Z

Clang was not pleased with this last commit, but it could be a fluke. I just deleted those results and we'll see if Marvin is happier on another try.

Myoldmopar · 2018-03-17T13:15:27Z

I merged in a couple more PRs so I went ahead and pulled develop in and pushed it back up. With the drastically reduced number of branches CI is watching, it should get done with a full set of diffs relatively soon this morning.

Myoldmopar · 2018-03-17T20:03:59Z

This looks to be good to go but I'm going to hold off from merging so the diffs don't get intermingled over on #6498. I'll merge this once the base Linux CI over there is done. Then we'll be nearly done...

Coil sizing report - fix plant index errors

920f84f

mjwitte added NewFeature Includes code to add a new feature to EnergyPlus NotIDDChange Code does not impact IDD (can be merged after IO freeze) labels Feb 26, 2018

mjwitte added 3 commits February 26, 2018 17:20

Coil sizing report - design document

d91df3e

Coil sizing report - I/O Ref changes add report to an example file

6f20750

Coil sizing report - Initial unit test

4b97a62

Myoldmopar reviewed Feb 28, 2018

View reviewed changes

mjwitte commented Feb 28, 2018

View reviewed changes

mjwitte added this to the EnergyPlus 8.9.0 milestone Mar 2, 2018

mjwitte added 3 commits March 13, 2018 12:57

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

39dc25e

…Followup # Conflicts: # src/EnergyPlus/WaterCoils.cc

Coil sizing report - fix build warnings and merge problems

dbb2559

Coil sizing report - modes to setCoilWaterFlow* and more unit tests

43728ca

mjwitte commented Mar 13, 2018

View reviewed changes

EnergyArchmage added 2 commits March 15, 2018 09:31

coil Summary report corrections

75fdccb

change zone weighting to supply-air-flow-rate from zone volume, traps to not overwrite values set directly

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

46a3952

…izingFollowup

EnergyArchmage added the DoNotPublish Includes changes that shouldn't be reported in the changelog label Mar 15, 2018

mjwitte mentioned this pull request Mar 15, 2018

New Coil Sizing Reports #6454

Merged

20 tasks

mjwitte removed the NewFeature Includes code to add a new feature to EnergyPlus label Mar 15, 2018

mjwitte added 2 commits March 15, 2018 18:06

Coil sizing report - more unit test coverage

73a655c

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

21aaea4

…Followup

mjwitte added 6 commits March 16, 2018 08:00

Fix lingering Coil:Heating:Gas --> Coil:Heating:Fuel

0fe9dd3

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

3b9e628

…Followup

Coil sizing report - more unit test coverage again

1d04a91

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

f618c7b

…Followup

Coil sizing report - table heading cleanup

a329a6b

Fix units in eio Surface Convection Parameters heading

e7ccfff

mjwitte requested a review from Myoldmopar March 16, 2018 14:39

mjwitte added Defect Includes code to repair a defect in EnergyPlus and removed DoNotPublish Includes changes that shouldn't be reported in the changelog labels Mar 16, 2018

mjwitte changed the title ~~New Coil Sizing Reports - Part 2~~ Fix lingering occurrences of Coil:Heating:Gas --> Fuel plus New Coil Sizing Reports Followup Mar 16, 2018

mjwitte commented Mar 16, 2018

View reviewed changes

mjwitte mentioned this pull request Mar 16, 2018

Fix VAV min flow frac > 1.0 and a bunch of termunitsizing issues related to multiple airloops #6338

Merged

13 tasks

Coil sizing report - revert table heading {TIMESTAMP} tags

e97f7b4

mjwitte commented Mar 16, 2018

View reviewed changes

Coil sizing report - revert table headings to KGWATER/KGDRYAIR

e245d68

mjwitte changed the title ~~Fix lingering occurrences of Coil:Heating:Gas --> Fuel plus New Coil Sizing Reports Followup~~ Change remaining Coil:Heating:Gas to Fuel plus cleanups on new coil sizing reports Mar 16, 2018

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

ce753f0

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

7d3213f

Myoldmopar merged commit 004ca8e into develop Mar 18, 2018

Myoldmopar deleted the CoilSizingFollowup branch March 18, 2018 00:25

Myoldmopar mentioned this pull request Mar 18, 2018

Additional Terminal Unit Sizing Issues #6481

Closed

11 tasks



		private: // data
		public: // data

Conversation

mjwitte commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Work Checklist

Review Checklist

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!

Myoldmopar commented Mar 16, 2018

Uh oh!

mjwitte commented Mar 16, 2018

Uh oh!

Myoldmopar commented Mar 16, 2018

Uh oh!

mjwitte commented Mar 16, 2018

Uh oh!

Myoldmopar commented Mar 16, 2018

Uh oh!

mjwitte commented Mar 16, 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 Mar 16, 2018

Uh oh!

mjwitte commented Mar 16, 2018

Uh oh!

mjwitte commented Mar 16, 2018

Uh oh!

Myoldmopar commented Mar 17, 2018

Uh oh!

Myoldmopar commented Mar 17, 2018

Uh oh!

Myoldmopar commented Mar 17, 2018

Uh oh!

Myoldmopar commented Mar 17, 2018

Uh oh!

Myoldmopar commented Mar 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mjwitte commented Feb 26, 2018 •

edited

Loading