Conversation
amirroth
added
Refactoring
|
This PR changes variable/meter and node numbering and broke all unit tests that rely on hard-coded numbers for these, a few for meters, many more for nodes. I have been able to fix most of these, but a few remain:
I tried a few fixes in one of the tests (I think the first one) and they didn't work for reasons that I couldn't parse.
Cannot get these to reproduce locally (mac arm64). Strangely, my local build had some other broken unit tests (e.g., in
This test uses very strange node number arithmetic that makes no sense to me. I tried to fix it both the way it was and the way I thought it should have been and neither of them worked. There is also a broken integration test that I need some advice on how to approach. |
|
@amirroth I fixed the AFN unit test that output the most useful information on the failure on Windows, the others will take a little bit more digging but seem to be somewhat similar so far. |
It looks like the coil was previously underspecified but wasn't hitting the validation in the test.
This one was failing on an infinite loop, there's protection against that now but may have unintended consequences.
|
|
jasondegraw
left a comment
jasondegraw
left a comment
There was a problem hiding this comment.
I've fixed the four failing AFN unit tests with changes that are mainly OK. I made a couple of comments on the fixes. The branch seems to have a whitespace problem, git was a bit confused about changes, but Windows may not have helped with that.
| }); | ||
|
|
||
| ASSERT_TRUE(process_idf(idf_objects)); | ||
| state->init_state(*state); |
There was a problem hiding this comment.
This change may run counter to the purpose of this branch, but I'll argue that the purpose of the unit test is (most likely) to check all of the AFN checks that happen in the test and not that the other parts of the model are set up correctly. Because they probably aren't.
| DuctSizingSBFlag = true; | ||
| } | ||
| while (NodeNum1 != NodeSplitter) { | ||
| bool foundNextDuct = false; |
There was a problem hiding this comment.
Added this boolean as part of a trapdoor to prevent an infinite loop failure. This came up in one of the failing unit tests, it then failed more normally, but the approach itself is risky.
|
@amirroth it has been 28 days since this pull request was last updated. |
| s_ip->getObjectDefMaxArgs(state, CurrentModuleObject, Count, NumAlphas, NumNumbers); | ||
| MaxNums = max(MaxNums, NumNumbers); | ||
| MaxAlps = max(MaxAlps, NumAlphas); | ||
| } |
There was a problem hiding this comment.
unit test failure SimulationManager_OutputDebuggingData, init_state was calling getScheduleInput and no schedules existed in unit test. Added check to avoid updating MaxNums/MaxAlps.
| if (s_sched->daySchedules.empty()) { | ||
| InitConstantScheduleData( | ||
| state); // why this was added: init_state crashed on the next line from call in unit test = SimulationManager_OutputDebuggingData | ||
| } |
There was a problem hiding this comment.
Unit test failure SimulationManager_OutputDebuggingData, the next line crashed (1276). Added this to setup constant schedules. Now next line does not crash.
| state.dataGlobal->KickOffSizing = true; | ||
| SetupZoneSizing(state, ErrorsFound); // Should only be done ONCE | ||
| SetupZoneSizing(state, ErrorsFound); // Should only be done ONCE | ||
| state.dataOutputProcessor->meterValues.resize(state.dataOutputProcessor->meters.size(), 0.0); // better way to do this for unit tests? |
There was a problem hiding this comment.
Unit test failure UnitHeater_HWHeatingCoilUAAutoSizingTest. Unit test calls UpdateMeterReporting, which allocates meterValues.size = 6, then calls SizingManager::ManageSizing which adds 42 more meters but does not allocate meterValues (because UpdateMeterReporting is not called again) and array bounds exceeded exception occurs. Probably a better way to do this but this does work.
| @@ -545,41 +548,38 @@ TEST_F(EnergyPlusFixture, MixedAir_HXBypassOptionTest) | |||
| state->dataAirSystemsData->PrimaryAirSystems(4).Branch(1).Comp(1).Name = "OA Sys 4"; | |||
| state->dataAirSystemsData->PrimaryAirSystems(4).Branch(1).Comp(1).TypeOf = "AirLoopHVAC:OutdoorAirSystem"; | |||
|
|
|||
There was a problem hiding this comment.
The following is update to previous changes to correct unit test failure MixedAir_HXBypassOptionTest. Frustrating but figured out what the nodes were and unit test now runs without exception.
| state->dataAirSystemsData->PrimaryAirSystems(4).Branch(1).Comp(1).TypeOf = "AirLoopHVAC:OutdoorAirSystem"; | ||
|
|
||
| int const OA5HeatCoilOutletNode = Util::FindItemInList( | ||
| "OA SYS 5 HC OUTLET NODE", state->dataLoopNodes->NodeID({1, state->dataLoopNodes->NumOfNodes}), state->dataLoopNodes->NumOfNodes); |
There was a problem hiding this comment.
i.e., node 18 was pointing to the heating coil outlet node. Only figured this out by running this unit test in develop. Added this to get actual node number and then used this number in several places below at lines 560, 579 and 688.
There was a problem hiding this comment.
4th unit test did not fail for me so no changes for that unit test:
There was a problem hiding this comment.
Also wanted to get these changes in before someone tries to resolve conflicts.
|
Only 1 unit test left to figure out. I ran ExerciseHVACDXHeatPumpSystem in debug and it passes. I ran again in release and it hung, did not finish did not crash, just hung. 
I added some printf statements to see how far it got and it passed in a release build. Go figure. 
I removed the printf statement and it hung again. |
|
|
…need to call SetupZoneSizing multiple times
…etup" This reverts commit 1021a14.
|
|
|
|
|
|
|
|
There was a problem hiding this comment.
This PR makes me somewhat uncomfortable. I think there's a case to be made that there's just too much going on here, (too much at stake if this one is wrong,) and that this should be broken into smaller chunks. On the other hand, nothing is obviously broken that I can see. There's are a LOT of diffs, but it looks to me like these are all somewhat expected given what this is doing.
I think this one in particular needs a very careful review before merging. @amirroth @jasondegraw @rraustad @jmarrec
| #include <cstdint> // std::uint8_t | ||
| #include <fstream> // std::ifstream |
There was a problem hiding this comment.
I'm not sure where these changes came from. Most of it is formatting, but develop is merged in, and this looks like part of the EnergyPlus::format conversion work. Probably safe to ignore.
| void CoilCoolingDX::registerComponentSets(EnergyPlusData &state) | ||
| { | ||
| auto *inputProcessor = state.dataInputProcessing->inputProcessor.get(); | ||
| auto const coilInstances = inputProcessor->epJSON.find(state.dataCoilCoolingDX->coilCoolingDXObjectName); | ||
| if (coilInstances == inputProcessor->epJSON.end() || coilInstances->empty()) { | ||
| return; | ||
| } | ||
|
|
||
| auto const &coilSchemaProps = inputProcessor->getObjectSchemaProps(state, state.dataCoilCoolingDX->coilCoolingDXObjectName); | ||
|
|
||
| for (auto const &coilInstance : coilInstances.value().items()) { | ||
| auto const &coilFields = coilInstance.value(); | ||
| std::string const coilName = Util::makeUPPER(coilInstance.key()); | ||
| std::string const evapInletNodeName = inputProcessor->getAlphaFieldValue(coilFields, coilSchemaProps, "evaporator_inlet_node_name"); | ||
| std::string const evapOutletNodeName = inputProcessor->getAlphaFieldValue(coilFields, coilSchemaProps, "evaporator_outlet_node_name"); | ||
|
|
||
| Node::TestCompSet(state, state.dataCoilCoolingDX->coilCoolingDXObjectName, coilName, evapInletNodeName, evapOutletNodeName, "Air Nodes"); | ||
| inputProcessor->markObjectAsUsed(state.dataCoilCoolingDX->coilCoolingDXObjectName, coilInstance.key()); | ||
| } |
There was a problem hiding this comment.
New helpers assist with setup.
| // Classify component sets by how the component/node relationship was discovered. | ||
| // Later validation only treats BranchTopology records as branch input, so the | ||
| // source name should describe that role. | ||
| auto const compSetSource = [&]() { | ||
| if (ParentTypeEnum == Node::ConnectionObjectType::Undefined) { | ||
| return Node::CompSetSource::ComponentRegistration; | ||
| } | ||
| if (ParentTypeEnum == Node::ConnectionObjectType::Branch) { | ||
| return Node::CompSetSource::BranchTopology; | ||
| } | ||
| return Node::CompSetSource::ParentChild; | ||
| }(); |
There was a problem hiding this comment.
Another helper to work out branch connections.
| // If parent type is undefined then no error | ||
| } | ||
| if ((ParentTypeEnum == Node::ConnectionObjectType::Undefined) || | ||
| (state.dataBranchNodeConnections->CompSets(Count).ParentObjectType == Node::ConnectionObjectType::Undefined)) { | ||
| // If node name is undefined then no error | ||
| } else if (InletNode != undefined) { | ||
| // If the matching node name does not belong to the parent or child object, then error | ||
| // For example a fan may share the same inlet node as the furnace object which is its parent | ||
| if ((ParentTypeEnum == state.dataBranchNodeConnections->CompSets(Count).ComponentObjectType) && | ||
| (ParentName == state.dataBranchNodeConnections->CompSets(Count).CName)) { | ||
| // OK - The duplicate inlet node belongs to this component's parent | ||
| } else if ((ComponentTypeEnum == state.dataBranchNodeConnections->CompSets(Count).ParentObjectType) && |
There was a problem hiding this comment.
There's a lot of if-then-else in this block that has been removed. I think all of the paths are accounted for now, but this in particular needs a careful review.
| // First time SimDXCoil is called, get the input for all the DX coils (condensing units) | ||
| if (state.dataDXCoils->GetCoilsInputFlag) { | ||
| GetDXCoils(state); | ||
| state.dataDXCoils->GetCoilsInputFlag = false; // Set GetInputFlag false so you don't get coil inputs again | ||
| } |
There was a problem hiding this comment.
Lots of these being removed.
| void resizeMeterValues(EnergyPlusData &state) | ||
| { | ||
| auto &op = state.dataOutputProcessor; | ||
| op->meterValues.resize(op->meters.size(), 0.0); | ||
| } |
There was a problem hiding this comment.
I think @rraustad had an open comment about cleaning these up that I need to look at. Maybe after this merges.
| void EnergyPlusFixture::clear_state_and_reset_err_stream() | ||
| { | ||
| state->clear_state(); | ||
| openOutputFiles(*state); | ||
|
|
||
| this->err_stream = new std::ostringstream; | ||
| state->files.err_stream = std::unique_ptr<std::ostream>(this->err_stream); | ||
|
|
||
| state->dataUtilityRoutines->outputErrorHeader = false; | ||
| } |
There was a problem hiding this comment.
Some tests were calling clear_state multiple times. The EnergyPlusFixture SetUp function already calls clear_state, but it also creates a new pointer for the err_stream and attaches it to state. Calling clear_state independently in the same test is 1) redundant and unnecessary if you only have one thing being tested, and 2) was not reattaching the pointer to the state err_stream. This second point was causing new failures, so this helper function was created and should be used if you really need to clear the state manually during tests.
| bool ErrorsFound; | ||
| int DataTotCapCurveIndex = 0; | ||
|
|
||
| state->dataDXCoils->GetCoilsInputFlag = false; |
There was a problem hiding this comment.
Lots and lots of one-off unit tests adjustments to make things work. I'm not going to go each specifically, nor do I know the reasons for them anyway.
|
|
|
@amirroth The PR description is fairly brief (albeit funny). Could you expand on the benefits you are trying to achieve by moving the |
|
@amirroth it has been 28 days since this pull request was last updated. |
1 similar comment
|
@amirroth it has been 28 days since this pull request was last updated. |
6 participants
The coil API refactor continues. Here, the various
GetCoilInputfunctions are moved toinit_state()and hilarity/madness ensues.