Remove FlowLock from plant components #10790
Conversation
mjwitte
added
the
Refactoring
| } | ||
| } | ||
| if (this->CondenserType == DataPlant::CondenserType::EvapCooled) { | ||
| this->EvapMassFlowRate = 0.0; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
This does look like a good change, but diffs from what?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I thought of a better way. Just set EvapMassFlowRate to the node flow here and delete the evap side SetComponentFlowRate call.
There was a problem hiding this comment.
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 . . .
There was a problem hiding this comment.
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.
|
| @@ -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; | |||
There was a problem hiding this comment.
Well that's even better. I didn't think of that. It takes a village.
| mdotCond = this->CondMassFlowRateMax; | ||
| } | ||
|
|
||
| PlantUtilities::SetComponentFlowRate(state, mdot, this->EvapInletNodeNum, this->EvapOutletNodeNum, this->CWPlantLoc); | ||
| PlantUtilities::SetComponentFlowRate(state, this->EvapMassFlowRate, this->EvapInletNodeNum, this->EvapOutletNodeNum, this->CWPlantLoc); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) {
|
| { | ||
| return (state.dataPlnt->PlantLoop(plantLoc.loopNum).LoopSide(plantLoc.loopSideNum).FlowLock != DataPlant::FlowLock::Unlocked && | ||
| !state.dataGlobal->WarmupFlag); | ||
| } |
There was a problem hiding this comment.
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?
|
|
|
@mjwitte @Myoldmopar it has been 28 days since this pull request was last updated. |
|
@mjwitte it has been 7 days since this pull request was last updated. |
Pull request overview
FlowLockfromokToIssueWarningthat checksWarmupFlagand component's loop sideFlowLock.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.