Remove FlowLock from plant components #10790

mjwitte · 2024-10-14T20:08:14Z

Pull request overview

Remove FlowLock from
- ChillerElectricEIR.cc
Add new worker function okToIssueWarning that checks WarmupFlag and component's loop side FlowLock.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

mjwitte · 2024-10-14T20:20:05Z

src/EnergyPlus/ChillerElectricEIR.cc

-            }
-        }
-        if (this->CondenserType == DataPlant::CondenserType::EvapCooled) {
+        this->EvapMassFlowRate = 0.0;


Had to add these zero lines to eliminate diffs for annual runs on the icestorage files (those were the first that came to mind where the chiller would be on a seriesactive branch).

This does look like a good change, but diffs from what?

Running this in the debugger, there are many times when it gets here to the no load block but EvapMassFlowRate is not zero, so calling SetComponentFlowRate with the non-zero flow changed the operation. Now, why isn't the flow rate zero before it gets here if there's no load, not sure. So maybe setting it to zero is a band-aid for something missed earlier?

EvapMassFlowRate appears to be the flow from the previous iteration. The node flow rate is set in initialize, but that's on the node data not this struct variable. So the node info should already be correct here? except that initialize uses std::abs(MyLoad). I guess it depends on how RunFlag gets set.

Real64 mdot = 0.0; Real64 mdotCond = 0.0; if ((std::abs(MyLoad) > 0.0) && RunFlag) { mdot = this->EvapMassFlowRateMax; mdotCond = this->CondMassFlowRateMax; } PlantUtilities::SetComponentFlowRate(state, mdot, this->EvapInletNodeNum, this->EvapOutletNodeNum, this->CWPlantLoc);

@rraustad Thanks, that explains the diffs. The old code would set the flow rates based on the node data. But when using SetComponentFlowRate the value of the flow when called is seen as a request, so if the node flow rate is zero but the max avail is not zero, then passing in a non-zero flow could result in the node flow rate being changed.

If you moved line 1869 to initialize, right after the code I show above, and changed it to this->EvapMassFlowRate = mdot then I think you can remove lines 1869 and 1870 since line 1869 was already called in initialize (I am a little worried about the difference in conditionals). That would just leave the clean up of the condenser flows as you've done below at lines 1872 - 1889. If no other changes are needed then just leave it since this is a good change as is.

I thought of a better way. Just set EvapMassFlowRate to the node flow here and delete the evap side SetComponentFlowRate call.

My understanding of the goal here is to let SetComponentFlowRate make the connection with the node, so setting it to the node flow rate would be back to the old code.

But your point is well taken that this shouldn't need a call to SetComponentFlowRate when load is zero, because that's already happened in initialize.

So, why is initialize using std::abs(MyLoad) > 0.0? To be consistent with the tests in calculate and update it should be MyLoad < 0.0.

And with the old code, I don't see anywhere that this->EvapMassFlowRate gets set to zero when the chiller is off.

And there are places that are setting node mass flow rates directly or reading from them, and messing with max/min avails etc. Hmm, this effort was just supposed to focus on eliminating FlowLock . . .

I think the original code (std::abs(MyLoad)) in init was just trying to force a positive value for load. But I was worried that MyLoad could be passed as a positive value in some cases and not be the same logic used in calc (i.e., might be diffs if you fixed it), I didn't track that down. I suspect there is a std::min(MyLoad, 0) in the plant manager for cooling components. So MyLoad < 0 should be the same as using abs. No reset of EvapMassFlowRate is probably an oversite. Scatter plotting that report variable and the node mass flow rate should give a perfect 45 degree slope. I'd venture to say that's not happening right now in some cases. Don't go too far with this, you've reached your goal. I was just trying to tighten this up a little to remove the unnecessary call to SetComponentFlowRate when no load/off condition exists, and how to get that done as efficient as possible. i.e., set EvapMassFlowRate right here in the if (MyLoad >= 0 || !RunFlag) { block instead of init so it's not always executed, it will get updated correctly later in calc if there is a load. Nothing really needs to change here because it's being done correctly now, it's just using that redundant call to match up EvapMassFlowRate with what happened in init which "could" be eliminated by reading the node. I also just noticed that the condenser water flow rate is also called in init so that node data is also already set correctly, no need for that function call either, and the condenser flow report variable is already zero'd at the top of calc (you could move that here). Reading the node has got to be faster than the function call(s). The min/max avail manipulation (I've done that myself in the past) should be done in SetComponentFlowRate because that's very important for the plant flow resolver, but that can be looked at later. I think I'm done now.

github-actions · 2024-10-15T22:50:43Z

⚠️ Regressions detected on macos-14 for commit `f8928bf`

Regression Summary

EIO: 22
ERR: 3
Table Big Diffs: 3
Table String Diffs: 1
ESO Big Diffs: 7
MTR Small Diffs: 18
ESO Small Diffs: 13
Table Small Diffs: 6

rraustad · 2024-10-16T15:51:24Z

src/EnergyPlus/ChillerElectricEIR.cc

@@ -1346,14 +1346,14 @@ void ElectricEIRChillerSpecs::initialize(EnergyPlusData &state, bool const RunFl
            state.dataLoopNodes->Node(state.dataPlnt->PlantLoop(this->CWPlantLoc.loopNum).TempSetPointNodeNum).TempSetPointHi;
    }

-    Real64 mdot = 0.0;
+    this->EvapMassFlowRate = 0.0;


Well that's even better. I didn't think of that. It takes a village.

mjwitte · 2024-10-16T15:53:20Z

src/EnergyPlus/ChillerElectricEIR.cc

        mdotCond = this->CondMassFlowRateMax;
    }

-    PlantUtilities::SetComponentFlowRate(state, mdot, this->EvapInletNodeNum, this->EvapOutletNodeNum, this->CWPlantLoc);
+    PlantUtilities::SetComponentFlowRate(state, this->EvapMassFlowRate, this->EvapInletNodeNum, this->EvapOutletNodeNum, this->CWPlantLoc);


Tried to set EvapMassFlowRate up in initialize but that starts introducing diffs. The chiller inlet node temp is slightly different from before in the first timestep when the chiller comes on. Note that just getting rid of abs here did not introduce any diffs. I'm running just the three icestorage files and getting diffs just in IceStorage-Parallel. CI might show other files with diffs.

I'm running annuals locally, so maybe CI won't show any diffs at all.

You didn't change anything else (and the abs isn't the cause). So why would using a report variable to make this initialization be any different than what was here originally? I wonder if it's this in update that is getting hit now?

if (this->CondMassFlowRate < DataBranchAirLoopPlant::MassFlowTolerance && this->EvapMassFlowRate < DataBranchAirLoopPlant::MassFlowTolerance) {

github-actions · 2024-10-16T16:33:37Z

⚠️ Regressions detected on macos-14 for commit `9ed9d07`

Regression Summary

EIO: 24
ERR: 3
Table Big Diffs: 4
Table String Diffs: 1
ESO Big Diffs: 7
MTR Small Diffs: 20
ESO Small Diffs: 15
Table Small Diffs: 7

rraustad · 2024-10-16T17:58:31Z

src/EnergyPlus/PlantUtilities.cc

+{
+    return (state.dataPlnt->PlantLoop(plantLoc.loopNum).LoopSide(plantLoc.loopSideNum).FlowLock != DataPlant::FlowLock::Unlocked &&
+            !state.dataGlobal->WarmupFlag);
+}


I don't see it in CppCheck results but it sure looks like the state argument can be declared as const? Maybe you tried that?

This reverts commit 0b330e4.

github-actions · 2024-10-18T20:15:17Z

⚠️ Regressions detected on macos-14 for commit `54a8d71`

Regression Summary

EIO: 2
ESO Small Diffs: 2
MTR Small Diffs: 2
Table Small Diffs: 1
Table Big Diffs: 1

…flowlock1

github-actions · 2024-10-18T22:28:53Z

⚠️ Regressions detected on macos-14 for commit `f307899`

Regression Summary

EIO: 1
ERR: 1
ESO Big Diffs: 1
Table Big Diffs: 1

nrel-bot-2c · 2024-11-16T00:20:14Z

@mjwitte @Myoldmopar it has been 28 days since this pull request was last updated.

nrel-bot-2 · 2024-11-23T00:18:09Z

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

github-actions · 2024-11-27T16:15:09Z

⚠️ Regressions detected on macos-14 for commit `0db3655`

Regression Summary

EIO: 1
ERR: 1
ESO Big Diffs: 1
Table Big Diffs: 1

mjwitte · 2024-12-06T21:49:04Z

src/EnergyPlus/ChillerElectricEIR.cc

+        case DataPlant::CondenserType::WaterCooled: {
+            this->CondMassFlowRate = 0.0;
+            PlantUtilities::SetComponentFlowRate(state, this->CondMassFlowRate, this->CondInletNodeNum, this->CondOutletNodeNum, this->CDPlantLoc);


This is inside if (MyLoad >= 0 || !RunFlag) so it's trying to tell the plant it has a zero flow request, but PlantUtilities::SetComponentFlowRate is returning a positive flow, because there's still flow on the chiller condenser inlet node at this point. But then later, plant flow resolution sets the inlet node mass flow rate to zero.

So this approach is causing this->CondMassFlowRate to be >0 at times when the chiller is off, because it never gets reset to zero, but the flow at the nodes is zero. I could just force this->CondMassFlowRate in the update function and maybe that's ok, but I'm just confused why PlantUtilities::SetComponentFlowRate is returning the inlet node flow rate when this is a simple branch with just the chiller condenser side on it.

This is for ASHRAE901_OfficeLarge_STD2019_Denver_ChillerEIR-Aug-Sep at just a few times when the chiller is going to start up after being off for a while, e.g. 08/02 18:15:00, 08/18 09:00:00, 08/20 07:45:00.

Tried adding this to the chiller update function when load is zero

this->CondMassFlowRate = 0.0; PlantUtilities::SetComponentFlowRate(state, this->CondMassFlowRate, this->CondInletNodeNum, this->CondOutletNodeNum, this->CDPlantLoc);

And there are times when this comes back as 54, because flow is locked here.

EnergyPlus/src/EnergyPlus/PlantUtilities.cc

Lines 270 to 272 in acda84e

} else if (loop_side.FlowLock == DataPlant::FlowLock::Locked) {

state.dataLoopNodes->Node(OutletNode).MassFlowRate = state.dataLoopNodes->Node(InletNode).MassFlowRate;

CompFlow = state.dataLoopNodes->Node(OutletNode).MassFlowRate;

A stab in the dark here. I wonder if there is a plant component (not necessarily the chiller) in the file your are testing that incorrectly sets one of these node mass flow rate variables (e.g., Min, Max, MinAvail, MaxAvail) such that these strange, unexpected results occur? Or are results like this more prevalent then initially thought but never seen because equipment is always available?

It's EMS! Chiller is called with a load, initialize requests flow on both the evaporator and condenser sides. The evaporator mass flow rate comes back zero, because EMS supervisory control has shut off this chiller's evap branch (note that the evap side flowlock is unlocked), and the condenser flow comes back at max flow as requested.

In calculate the zero evap mass flow rate results in MyLoad=0 and return without resetting the condender flow rate to zero. But even setting that to zero and calling setcomponentflowrate doesn't work, because the condenser side is already flowlocked at the earlier flow request.

In update if MyLoad ==0 it forces zero flow on the condenser nodes without using setcomponentflowrate (which wouldn't work anyway because the condenser side is locked at max flow.

That's enough twists and turns for today.

So then the condenser plant FlowLock needs to be conditional on the FlowLock for the plant it serves?

mjwitte added 3 commits October 14, 2024 14:29

ChillerElectricEIR remove flowlock for no load flow

d56f211

New okToIssueWarning function checks FlowLock

8560a93

ChillerElectricEIR remove flowlock for warnings

37e676b

mjwitte added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Oct 14, 2024

mjwitte commented Oct 14, 2024

View reviewed changes

mjwitte added 2 commits October 15, 2024 13:37

Merge remote-tracking branch 'remotes/origin/develop' into flowlock1

08a5f61

ChillerReformulatedEIR remove flowlock

0b330e4

mjwitte added 2 commits October 16, 2024 07:39

Merge remote-tracking branch 'remotes/origin/develop' into flowlock1

04e9107

ChillerElectricEIR set EvapMassFlowRate in initialize

22e12fe

rraustad reviewed Oct 16, 2024

View reviewed changes

mjwitte commented Oct 16, 2024

View reviewed changes

rraustad reviewed Oct 16, 2024

View reviewed changes

Revert "ChillerReformulatedEIR remove flowlock"

03fce6d

This reverts commit 0b330e4.

mjwitte added 2 commits October 18, 2024 16:08

Merge remote-tracking branch 'remotes/origin/chillerEIRrepairs' into …

89d94b4

…flowlock1

Address const comment

290bf6e

Merge remote-tracking branch 'remotes/origin/develop' into flowlock1

7cecb90

Merge remote-tracking branch 'remotes/origin/develop' into flowlock1

0001b6b

mjwitte commented Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove FlowLock from plant components #10790

Remove FlowLock from plant components #10790

mjwitte commented Oct 14, 2024

mjwitte Oct 14, 2024

rraustad Oct 14, 2024

mjwitte Oct 14, 2024

rraustad Oct 14, 2024

mjwitte Oct 15, 2024

rraustad Oct 15, 2024

rraustad Oct 16, 2024

mjwitte Oct 16, 2024

rraustad Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024

rraustad Oct 16, 2024

mjwitte Oct 16, 2024 •

edited

Loading

rraustad Oct 16, 2024

github-actions bot commented Oct 16, 2024

rraustad Oct 16, 2024

github-actions bot commented Oct 18, 2024

github-actions bot commented Oct 18, 2024

nrel-bot-2c commented Nov 16, 2024

nrel-bot-2 commented Nov 23, 2024

github-actions bot commented Nov 27, 2024

mjwitte Dec 6, 2024 •

edited

Loading

mjwitte Dec 6, 2024

rraustad Dec 6, 2024

mjwitte Dec 9, 2024

rraustad Dec 11, 2024

	} else if (loop_side.FlowLock == DataPlant::FlowLock::Locked) {
	state.dataLoopNodes->Node(OutletNode).MassFlowRate = state.dataLoopNodes->Node(InletNode).MassFlowRate;
	CompFlow = state.dataLoopNodes->Node(OutletNode).MassFlowRate;

Remove FlowLock from plant components #10790

Are you sure you want to change the base?

Remove FlowLock from plant components #10790

Conversation

mjwitte commented Oct 14, 2024

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rraustad Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 15, 2024

⚠️ Regressions detected on macos-14 for commit f8928bf

Choose a reason for hiding this comment

mjwitte Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2024

⚠️ Regressions detected on macos-14 for commit 9ed9d07

Choose a reason for hiding this comment

github-actions bot commented Oct 18, 2024

⚠️ Regressions detected on macos-14 for commit 54a8d71

github-actions bot commented Oct 18, 2024

⚠️ Regressions detected on macos-14 for commit f307899

nrel-bot-2c commented Nov 16, 2024

nrel-bot-2 commented Nov 23, 2024

github-actions bot commented Nov 27, 2024

⚠️ Regressions detected on macos-14 for commit 0db3655

mjwitte Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rraustad Oct 16, 2024 •

edited

Loading

⚠️ Regressions detected on macos-14 for commit `f8928bf`

mjwitte Oct 16, 2024 •

edited

Loading

⚠️ Regressions detected on macos-14 for commit `9ed9d07`

⚠️ Regressions detected on macos-14 for commit `54a8d71`

⚠️ Regressions detected on macos-14 for commit `f307899`

⚠️ Regressions detected on macos-14 for commit `0db3655`

mjwitte Dec 6, 2024 •

edited

Loading