-
Notifications
You must be signed in to change notification settings - Fork 460
Fix order of curve independent variables for GAHP #11071
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
Conversation
|
|
||
| // evaluate capacity modifier curve and determine load side heat transfer | ||
| Real64 capacityModifierFuncTemp = Curve::CurveValue(state, this->capFuncTempCurveIndex, waterTempforCurve, oaTempforCurve); | ||
| Real64 capacityModifierFuncTemp = Curve::CurveValue(state, this->capFuncTempCurveIndex, oaTempforCurve, waterTempforCurve); |
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.
I think I would have voted for a change in documentation since all other models use Tamb as the second independent variable. It doesn't really matter except for consistency across models. I do see that the paper used Tamb as the first independent variable but when calculating biquadratic regression coefficients that order is subjective. It just means that the paper's coefficient a and b would become c and d in E+. Does the example file use these same coefficients? @mjwitte what are your thoughts on independent variable order for performance curves?
EIR HP:
capacityModifierFuncTemp = Curve::CurveValue(state, this->capFuncTempCurveIndex, loadSideOutletSetpointTemp, this->sourceSideInletTemp);
DXCoils:
TotCapTempModFac = CurveValue(state, thisDXCoil.CCapFTemp(Mode), InletAirWetBulbC, CondInletTemp);
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.
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.
@mjwitte - The example file includes curves with only a constant coefficient:
Curve:Biquadratic,
EIRCurveFuncTemp, !- Name
1.0, !- Coefficient1 Constant
0.0, !- Coefficient2 x
0.0, !- Coefficient3 x**2
0.0, !- Coefficient4 y
0.0, !- Coefficient5 y**2
0.0, !- Coefficient6 x*y
5.0, !- Minimum Value of x
10.0, !- Maximum Value of x
24.0, !- Minimum Value of y
35.0, !- Maximum Value of y
, !- Minimum Curve Output
, !- Maximum Curve Output
Temperature, !- Input Unit Type for X
Temperature, !- Input Unit Type for Y
Dimensionless; !- Output Unit Type
Curve:Biquadratic,
CapCurveFuncTemp, !- Name
1.0, !- Coefficient1 Constant
0.0, !- Coefficient2 x
0.0, !- Coefficient3 x**2
0.0, !- Coefficient4 y
0.0, !- Coefficient5 y**2
0.0, !- Coefficient6 x*y
5.0, !- Minimum Value of x
10.0, !- Maximum Value of x
24.0, !- Minimum Value of y
35.0, !- Maximum Value of y
, !- Minimum Curve Output
, !- Maximum Curve Output
Temperature, !- Input Unit Type for X
Temperature, !- Input Unit Type for Y
Dimensionless; !- Output Unit Type
So, should I revert these changes and make the changes in the documentation? Here's perhaps another argument for modifying the documentation instead of the code: the EIR-f-T curves in the paper have to be renormalized because they describe not just a EIR modifier but the change in rated EIR as a function of the temperature. Please, let me know what you think is best.
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.
I guess changing the documentation (and replacing these constant curves) is the better option. Sorry to send you down the wrong path.
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.
Got it, no worries.
|
|
Myoldmopar
left a comment
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.
Looks like this is settled now. Docs and example file and code agree. Any final thoughts here before merging?
|
So I see in the percdiff.csv a change of about 20% in load side heat transfer and roughly the same in fuel rate for the first few hours. I didn't run the example file so maybe this makes sense, for a water temp pull up or something during this time. And then during the day there is no change in load side heat transfer or inlet/outlet water temp or fuel rate, I can't tell if the unit is running or not but if it is, and the coefficients were changed, why isn't there diffs at this time? Then at the end of the day there is only a change in fuel rate? Is the unit running? I suspect it is running and the "load" is the same as before but the fuel use has changed. It's what you can't see in diffs that bring up more questions. |
|
OK, so #11068 should go in before this, right? In my mind, the first PR was causing an issue, so a follow-up PR was going to tackle the diffs....but the numbering doesn't match up with that. I'm just wanting to confirm, and I can pull develop into the "second" PR to get cleaner results. |
|
@Myoldmopar - That is correct. Although because the changes in this PR are now only documentation changes, I don't think that it matters since it shouldn't show any diffs. I think that the diffs that Rich was looking at were from the original proposed changes which involved code modifications instead of documentation modifications. |
|
Ahh, that's right. Cool, OK, merging this then. Thanks. |


Pull request overview
Description of the purpose of this PR
Fix multi-variate curve lookups to be in-line with the documentation.
Pull Request Author
Reviewer