Skip to content

Conversation

lymereJ
Copy link
Collaborator

@lymereJ lymereJ commented Sep 30, 2025

Pull request overview

Description of the purpose of this PR

Pull Request Author

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions

Reviewer

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@lymereJ lymereJ added Defect Includes code to repair a defect in EnergyPlus AirflowNetwork Related primarily on airflow-network portions of the codebase labels Sep 30, 2025
Copy link
Collaborator Author

@lymereJ lymereJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Walk-through:

return state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).PartLoadRatio;
}

void SetVarSpeedDXCoilAirLoopNumber(EnergyPlusData &state, std::string const &CoilName, int const AirLoopNum)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New function to associate airloop index to variable speed coil.

Comment on lines 6534 to 6540
state->dataVariableSpeedCoils->GetCoilsInputFlag = false;
state->dataVariableSpeedCoils->VarSpeedCoil.allocate(2);
state->dataVariableSpeedCoils->VarSpeedCoil(1).Name = "Super Coil";
state->dataVariableSpeedCoils->VarSpeedCoil(2).Name = "Super Heating Coil";

state->afn->validate_distribution();
compare_err_stream("");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adapt existing unit test to check for the error stream.

@lymereJ lymereJ added this to the EnergyPlus 25.2 milestone Sep 30, 2025
@lymereJ lymereJ marked this pull request as ready for review September 30, 2025 21:10
ErrorsFound = true;
} else {
SetDXCoilAirLoopNumber(m_state, DisSysCompCoilData(i).name, DisSysCompCoilData(i).AirLoopNum);
SetVarSpeedDXCoilAirLoopNumber(m_state, DisSysCompCoilData(i).name, DisSysCompCoilData(i).AirLoopNum);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well obviously!


WhichCoil = Util::FindItemInList(CoilName, state.dataVariableSpeedCoils->VarSpeedCoil);
if (WhichCoil != 0) {
state.dataVariableSpeedCoils->VarSpeedCoil(WhichCoil).AirLoopNum = AirLoopNum;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where exactly is this used? AirFlowNetwork?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, how does the VS coil use this information? AirflowNetwork already knows the AirLoopNum and is passing that number to the coil model. Why does the coil need to know the AirLoopNum?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. The DXCoil model uses that information like this. I don't see either of these variables set in the VS coil model.

state.dataAirLoop->LoopDXCoilRTF = max(thisDXCoil.CoolingCoilRuntimeFraction, thisDXCoil.HeatingCoilRuntimeFraction);
if (thisDXCoil.AirLoopNum > 0) {
    state.dataAirLoop->AirLoopAFNInfo(thisDXCoil.AirLoopNum).AFNLoopDXCoilRTF =
        max(thisDXCoil.CoolingCoilRuntimeFraction, thisDXCoil.HeatingCoilRuntimeFraction);
}

So the VS coil model would also have to do things like this so that AFN has that information.

Copy link
Contributor

@rraustad rraustad Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is strange with these functions that set the air loop num in a coil model, is that when the coil is simulated the global state.dataSize->CurSysNum is always set when air loops are simulated. So the coil model already knows the AirLoopNum associated with this coil. These functions aren't even needed. Now it may be tricky to internally set up this coil variable based on whether or not AFN is active but I think this could be done as a one-time check in init. Anyway, this is the way it's done now so no reason to go out of scope for this issue. I still think there is more work in the VS coil model before this would work as AFN intends (e.g., the coil model needs the code I show above). I think that's the only other thing needed in the VS coil code.

image image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rraustad Airloopnum is also used for reporting the airloop name to some of the table reports (but I'm not sure it's consistent across all of the coil types).
And then there's the coil selection report that has it's own tracking of the coil airloopnum and sets this is (nearly?) every ReportCoilSelection::setCoil* function:
c->airloopNum = curSysNum;.

And here's some of the table output for the HeatPumpAuto example file. The first two tables don't know the airloop name, but the next one does?
image

And then later, we have other reports that show the airloop name
image

And equivalent tables for heating coils do not even report airloop name.
image

@rraustad I agree that every coil should simply store it's airloop num from curSysNum at the top of its sizing function. Maybe there's a timing issue for the AFN usage?

@lymereJ Maybe we can fix the "N/A" in the "DX Heating Coils" tables in this PR by adding thisDXCoil.AirLoopNum=state.dataSize->CurSysNum at the top of SizeDXCoil?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lymereJ @rraustad Oh, but now I see and AirloopNum>0 is being used to tell the DX coil if it's in an AFN loop?!
Then setting AirLoopNum could break things. That should really be a separate flag that gets set from AFN on the coil.

I'll work on a separate PR to fix the table (just use CurSysNum instead of the coil's AirLoopNum). And I'll check other coil types to see if any others are assuming (incorrectly) that AirLoopNum (or equivalent) is set properly for non-AFN runs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting AirloopNum to know if AFN is active is kind of klunky because that data is also used for table reporting. Whatever the AFN variables are then if AFN == Active && AirloopNum > 0 would be better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting AirloopNum to know if AFN is active is kind of klunky because that data is also used for table reporting. Whatever the AFN variables are then if AFN == Active && AirloopNum > 0 would be better.

Can we assume if there's AFN with distribution, then all coils are part of that. Or should the current AFN function calls to the coils be used to set a coil-specific AFN flag?

Comment on lines 198 to 200
if (varSpeedCoil.AirLoopNum > 0) {
state.dataAirLoop->AirLoopAFNInfo(varSpeedCoil.AirLoopNum).AFNLoopDXCoilRTF = max(varSpeedCoil.RunFracCool, varSpeedCoil.RunFracHeat);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding missing AFNLoopDXCoilRTF assignment for variable speed coils as pointed out by @rraustad, here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AirflowNetwork Related primarily on airflow-network portions of the codebase Defect Includes code to repair a defect in EnergyPlus

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AirflowNetwork validation issue for variable speed coils

5 participants