Add performance support for ASHRAE205 Coils (RS0004) #10307
Conversation
…yplus into ashrae-205-coils
…isn't detected by libtk205.
|
@tanaya-mankad @Myoldmopar it has been 28 days since this pull request was last updated. |
|
@tanaya-mankad it has been 9 days since this pull request was last updated. |
|
@tanaya-mankad it has been 7 days since this pull request was last updated. |
5 similar comments
|
@tanaya-mankad it has been 7 days since this pull request was last updated. |
|
@tanaya-mankad it has been 7 days since this pull request was last updated. |
|
@tanaya-mankad it has been 7 days since this pull request was last updated. |
|
@tanaya-mankad it has been 7 days since this pull request was last updated. |
|
@tanaya-mankad it has been 7 days since this pull request was last updated. |
|
@tanaya-mankad @Myoldmopar it has been 29 days since this pull request was last updated. |
|
@Myoldmopar @tanaya-mankad @Myoldmopar it has been 28 days since this pull request was last updated. |
|
@tanaya-mankad @nealkruis I merged in develop and resolved the conflicts. There were a few, all related to formatting changes with our bump to newer Clang Format and brace rules. It is passing happily locally, so assuming CI is happy, this should be ready to merge later today. |
Myoldmopar
left a comment
There was a problem hiding this comment.
OK, this is looking good. I have a few comments that I'd like resolved before merging.
- There are some stray
doublethat should beReal64to fit with E+ - There appears to be a stray .bak backup file in the branch that needs to be cleaned out
- I'd like to hear your thoughts on the shared pointer necessity
Honestly, this is super close though.
| Real64 air_mass_flow_rate) const; | ||
|
|
||
| // Rating constants | ||
| const double outdoor_coil_entering_dry_bulb_temperature_K{308.15}; // 95F |
There was a problem hiding this comment.
Is this .bak file an accidental commit? I'm assuming it's some backup file.
| int condInletNodeIndex = 0; | ||
| int condOutletNodeIndex = 0; | ||
| CoilCoolingDXCurveFitPerformance performance; | ||
| std::shared_ptr<CoilCoolingDXPerformanceBase> performance; // TODO: unique_ptr and explicit copy ctor |
There was a problem hiding this comment.
I'm not sure about this being needed. If you want to try to convince me, I'm open to hearing opinions as to why it's necessary.
There was a problem hiding this comment.
Since the performance is now decided at runtime (through the IDF), we decided to implement it with runtime polymorphism. It seems like the commonest method, so we added a Performance base class and derive 205 and standard curve-fit performance from that. Possibly I made some obvious oversight of how to implement this?
There was a problem hiding this comment.
We've just had lots of debates about whether we need the extra layers of managed pointers within EnergyPlus. The argument is usually that we do not dynamically allocate anything throughout the program, we just stand everything up once at init, and then run, and then leave. And in cases like PlantComponent, we have each component stored in a regular container on state, and then we operate on raw non-owning pointers to the common base class of those components to capture polymorphism.
I don't think it's necessarily worth holding this up. Just thought I'd ask.
Myoldmopar
left a comment
There was a problem hiding this comment.
Looks good to me. One final test pass locally (redundant, but still), and then this should be ready to go.
| \paragraph{Field: Performance Interpolation Method} | ||
|
|
||
| This alpha field specifies how performance data from the Representation File will be interpolated. | ||
| Choices are "Linear" and "Cubic." |
There was a problem hiding this comment.
For the record, in LaTeX, we prefer ``backticks then apostrophes'' and let TeX render them as it sees fit. Don't need to hold this up for it.
|
Yep, all happy here. Merging this. Thanks @tanaya-mankad and @nealkruis |
Pull request overview
This code implements Coil Performance as an inheritable base class, which allows the conventional curve-based performance or a new ASHRAE205-compliant performance to be generated at runtime from the IDF information.
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.