-
Notifications
You must be signed in to change notification settings - Fork 394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Absorption Chiller Plant Component Refactor #7639
Conversation
Ready for review. |
|
||
extern Array1D_bool CheckEquipName; | ||
|
||
extern bool GetInput; // When TRUE, calls subroutine to read input file | ||
extern bool getInput; // When TRUE, calls subroutine to read input file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for later, a lot of times these getInput flags aren't actually needed to be defined and exposed via the header file, it can be defined at the top of the cc file only, so just watch out for that on later refactors. Note there are occasions where this isn't actually true, so I'm just throwing this out there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Myoldmopar thanks. This is externed because FaultsManager uses it.
src/EnergyPlus/PlantLoopEquip.cc
Outdated
sim_component.SizFac = SizingFac; | ||
} | ||
|
||
dynamic_cast<ChillerAbsorption::BLASTAbsorberSpecs*> (sim_component.compPtr)->EquipFlowCtrl = EquipFlowCtrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to just set this inside the function instead. There are examples of this but I can't think of one at the moment. Basically, in the oneTimeInit function, or the simulate function, get the flow control value off the plant structure:
this->EquipFlowCtrl = DataPlant::PlantLoop(componentLocation.LoopNum).LoopSide(...blah... .EquipFlowCtrl;
This avoids the dynamic cast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Myoldmopar fixed, and develop is pulled in.
@@ -313,6 +310,7 @@ void EnergyPlusFixture::clear_all_states() | |||
BoilerSteam::clear_state(); | |||
BranchInputManager::clear_state(); | |||
CoolingPanelSimple::clear_state(); | |||
ChillerAbsorption::clear_state(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always a great sign!
@mitchute could you comment on my two notes above. If you can address those, and pull develop in, it would be great to get one more clean build and then this can go in. |
Refactors for absorption chiller, indirect absorption chiller, swimming pool, and central GSHP were all merged down locally and tested at once. The only diff was in the indirect, where the missing reset on the variable was causing an expected diff. All four PRs will merge at once here to start clearing the path for CI. |
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.