Skip to content

Conversation

@lymereJ
Copy link
Collaborator

@lymereJ lymereJ commented May 19, 2025

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

  • 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 any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • 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 the Defect Includes code to repair a defect in EnergyPlus label May 19, 2025
@lymereJ lymereJ added this to the EnergyPlus 25.2 IO Freeze milestone May 19, 2025

// 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);
Copy link
Contributor

@rraustad rraustad May 20, 2025

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?

image

EIR HP:
capacityModifierFuncTemp = Curve::CurveValue(state, this->capFuncTempCurveIndex, loadSideOutletSetpointTemp, this->sourceSideInletTemp);

DXCoils:
TotCapTempModFac = CurveValue(state, thisDXCoil.CCapFTemp(Mode), InletAirWetBulbC, CondInletTemp);

Copy link
Contributor

Choose a reason for hiding this comment

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

@rraustad My initial advice was to follow the paper, but I didn't think about how other similar curves are done in EnergyPlus.
@lymereJ I don't see any changes here to the example file. Can you tell which variable order was used in those curves?

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, no worries.

@github-actions
Copy link

⚠️ Regressions detected on macos-14 for commit 7e615e6

Regression Summary
  • Audit: 1
  • ESO Big Diffs: 1

@github-actions
Copy link

⚠️ Regressions detected on macos-14 for commit cc2db57

Regression Summary
  • Audit: 1
  • ESO Big Diffs: 1

@lymereJ lymereJ marked this pull request as ready for review May 21, 2025 16:24
Copy link
Member

@Myoldmopar Myoldmopar left a 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?

@rraustad
Copy link
Contributor

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.

image

@lymereJ
Copy link
Collaborator Author

lymereJ commented May 21, 2025

@rraustad - There's another PR ready for review #11068 which I think tackle some of that seemingly on/off behavior of the unit. Please, let me know what you think.

@lymereJ lymereJ changed the title Fix order of curve independent variables for GAHP. Fix order of curve independent variables for GAHP May 21, 2025
@Myoldmopar
Copy link
Member

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.

@lymereJ
Copy link
Collaborator Author

lymereJ commented May 27, 2025

@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.

@Myoldmopar
Copy link
Member

Ahh, that's right. Cool, OK, merging this then. Thanks.

@Myoldmopar Myoldmopar merged commit 931ef2e into develop May 27, 2025
9 checks passed
@Myoldmopar Myoldmopar deleted the fix_gahp_curve_lookups branch May 27, 2025 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Defect Includes code to repair a defect in EnergyPlus

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gas-Fired absorption heat pump curve lookups use independent variables in the wrong order

6 participants