Optimize TableEntry Handling#11469
Conversation
amirroth
added
Refactoring
amirroth
left a comment
amirroth
left a comment
There was a problem hiding this comment.
Simple walk through.
| s->pdchSHGSHtOtherRem = newPreDefColumn(state, s->pdstSHGSpkHt, "Opaque Surface Conduction and Other Heat Removal [W]"); | ||
|
|
||
| // Standard62Report | ||
| if (state.dataGlobal->DoZoneSizing || state.dataGlobal->DoSystemSizing) { |
There was a problem hiding this comment.
Don't condition constant initialization based on flags. Initialize always and use the flags to condition display later.
| // SUBROUTINE PARAMETER DEFINITIONS: | ||
| assert(columnIndex > 0 && columnIndex <= state.dataOutRptPredefined->numColumnTag); | ||
| auto &column = state.dataOutRptPredefined->columnTag(columnIndex); | ||
| auto &table = state.dataOutRptPredefined->subTable(column.indexSubTable); |
There was a problem hiding this comment.
Store entries in table-specific lists.
| auto &table = state.dataOutRptPredefined->subTable(column.indexSubTable); | ||
|
|
||
| for (int i = 1; i <= table.numEntries; ++i) { | ||
| if (table.entries(i).indexColumn == columnIndex && table.entries(i).objectName == objName) { |
There was a problem hiding this comment.
Search entries only from the appropriate table.
| int numEntries; | ||
| int sizeEntries; | ||
|
|
||
| Array1D<TableEntryType> entries; |
There was a problem hiding this comment.
Table-specific entry lists.
|
|
||
| for (auto ¤tStyle : ort->tabularReportPasses) { | ||
|
|
||
| // loop through the entries and associate them with the subtable and create |
There was a problem hiding this comment.
We will do this on a per-table basis below.
| } | ||
| } | ||
|
|
||
| int curNumRows = numUniqueObjectNames; |
There was a problem hiding this comment.
Number of rows equals number of unique objects.
| } | ||
|
|
||
| // determine what row the current entry is in | ||
| int const rowCurrent = entry.uniqueObjName; |
There was a problem hiding this comment.
Indentation has changed, that is all.
|
|
||
| TEST_F(EnergyPlusFixture, OutputReportTabularTest_PredefinedTableRowMatchingTest) | ||
| { | ||
|
|
There was a problem hiding this comment.
This is being called by init_constant_state().
|
|
||
| void init_constant_state([[maybe_unused]] EnergyPlusData &state) override | ||
| { | ||
| OutputReportPredefined::SetPredefinedTables(state); |
There was a problem hiding this comment.
Call SetPredefinedTables() here.
| // Do this some other way, whether or not a report is used should not depend on whether or not it is defined | ||
|
|
||
| GetInputOutputTableSummaryReports(*state); | ||
| // auto &reportNameArray = state->dataOutRptPredefined->reportName; |
There was a problem hiding this comment.
Disable this part of the unit test (for now?). Maybe do this via an #ifdef DISABLED.
|
|
|
|
|
|
|
I've tested this locally and am seeing that for files with no system sizing (ASIHPMixedTank.idf, for example), the 62.1 table entries appear in the .htm file, but they are blank. Whereas in the baseline branch, they do not appear at all, which I think is consistent with what you described regarding the unit test. Hence some of the very large .audit file diffs. sizeReportName= 100
- numReportName= 18
+ numReportName= 19
sizeSubTable= 200
- numSubTable= 105
+ numSubTable= 113
sizeColumnTag= 1600
- numColumnTag= 968
- sizeTableEntry= 6400
+ numColumnTag= 1053
+ sizeTableEntry= 7900
numTableEntry= 3248For files with system sizing (5ZoneAirCooledConvCoeff.idf), I am still seeing very large .audit file diffs, but the .htm files show no numerical diffs (though the entries are reordered in some cases). - sizeTableEntry= 6400
+ sizeTableEntry= 11200 |
The audit files diffs don't really matter. Especially the ones having to do with sizes of lists as opposed to number of entries. The blank table is a problem, but it needs a different solution than what was previously used. I can try to come up with something, or maybe @JasonGlazer has an idea. |
See old and new line 1450 of OutputReportPredefined.cc, these tables are now created regardless if sizing is selected. |
| s->pdchS62shdEvz = newPreDefColumn(state, s->pdstS62sysHeatDes, "Zone Ventilation Efficiency - Evz-min"); | ||
| } | ||
|
|
||
| // if (state.dataGlobal->DoZoneSizing || state.dataGlobal->DoSystemSizing) { |
There was a problem hiding this comment.
This line is the reason the tables are now present and blank.
|
This brings back the missing tables, but there's one large math diff I haven't gotten to the bottom of. If you'd prefer something else, we can revert the last commit. |
|
|
|
@amirroth it has been 28 days since this pull request was last updated. |
|
|
|
|
|
|
|
I believe this is ready for review. Lots of .aud diffs, but I'm not sure if we care about that. This from 
Thoughts? @JasonGlazer pointed out that we are seeing some performance benefits. 
|
rraustad
left a comment
rraustad
left a comment
There was a problem hiding this comment.
The only diff is audit files and those diffs have been explained. I am inclined to merge.
…d by the test fixture
mitchute
left a comment
mitchute
left a comment
There was a problem hiding this comment.
A few final comments. I think this can merge soon if tests pass and I don't hear from anyone else.
| SetPredefinedTables(*state); | ||
|
|
There was a problem hiding this comment.
init_constant_state() was being called by the test fixture, so it's no longer necessary to call this explicitly in each test.
| void clearDXCoolingCoilStandardRatingTables(EnergyPlusData &state) | ||
| { | ||
| auto &orp = *state.dataOutRptPredefined; | ||
| for (int subTableIndex : {orp.pdstDXCoolCoil, orp.pdstDXCoolCoil_2023}) { | ||
| auto &subTable = orp.subTable(subTableIndex); | ||
| subTable.entries.deallocate(); | ||
| subTable.numEntries = 0; | ||
| subTable.sizeEntries = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Added a test helper here, since there are two tests that had previously called SetPredefinedTables multiple times to set/check/reset table values.
|
|
4 participants
This PR optimizes
TableEntryhandling by storing entries in table-specific lists instead of a single global list. When printing out tables, each entry has to be traversed only once rather than once per table. Unique rows are also tracked on a per-table basis.To enable this,
SetPredefinedTableswas moved toinit_constant_state(). This broke a unit test which checked that the Standard 62.1 was not being printed out if sizing was not done. This logic was implemented by not initializing the table constants ifSizingwas off, but that's not the way to do something like this. The unit test is partially disabled for now.