Skip to content
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

Addresses #4946, allow TableLookup curves for Coil:*:WaterToAirHeatPump:EquationFit objects #4950

Merged
merged 12 commits into from Sep 26, 2023

Conversation

joseph-robertson
Copy link
Collaborator

Pull request overview

Pull Request Author

  • Model API Changes / Additions
  • Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
  • Model API methods are tested (in src/model/test)
  • EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
  • If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
  • If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
  • Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
  • All new and existing tests passes
  • If methods have been deprecated, update rest of code to use the new methods

Labels:

  • If change to an IDD file, add the label IDDChange
  • If breaking existing API, add the label APIChange
  • If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • Code Style, strip trailing whitespace, etc.
  • All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
  • Labeling is ok
  • 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

@joseph-robertson joseph-robertson added Enhancement Request component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. APIChange A motivated non-backward compatible change that breaks the existing API and needs to be communicated labels Aug 29, 2023
@joseph-robertson joseph-robertson self-assigned this Aug 29, 2023
}

bool CoilHeatingWaterToAirHeatPumpEquationFit_Impl::setHeatingCapacityCurve(const CurveQuadLinear& heatingCapacityCurve) {
bool CoilHeatingWaterToAirHeatPumpEquationFit_Impl::setHeatingCapacityCurve(const Curve& heatingCapacityCurve) {
Copy link
Collaborator

@jmarrec jmarrec Aug 30, 2023

Choose a reason for hiding this comment

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

This specifically expects a QuadvariateFunctions one in the IDD. Should we check that heatingCapacityCurve.numVariables() == 4 here?

Genuine question. Not saying we should necessarily, but since this is the general move of E+ to allow generic curve and table objects everywhere, this is bound to come up again, I think now's a good time to decide what to do/standardize. @kbenne @joseph-robertson

Pros:

  • Checking is good because it catches user errors early instead of letting E+ complain (via CheckCurveDim)

Cons:

  • More work for us
  • Options:
    • Option1: we are hardcoding a curve.numVariables() check in each setter that takes a generic Curve, which potentially could fall out of date (though in this case I don't see why the number of expected independent variables would change suddenly in E+)
    • Option 2: The alternative is to add a special case in WorkspaceObject_Impl::setPointer or a new method WorkspaceObject_Impl::setCurvePointer
      • A new check in setPointer is going to impose a performance penalty for all objects
      • A new method specifically for Curves means we gotta be careful to use this one specifically for curves

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually for option 2, the best option is probably to a new bool ModelObject_Impl::setCurve(unsigned index, const Curve& curve) method that would check the IDD references and do the numVariables() check for us before setting the handle via WorkspaceObject_Impl::setPointer (or WorkspaceObject_Impl::setString for speed).

That'd avoid having to do lots of shenanigans in Workspace land to figure out the number of variables a Table object has.

Copy link
Contributor

@kbenne kbenne Sep 25, 2023

Choose a reason for hiding this comment

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

Increasingly, I'm starting to appreciate some value in not making OpenStudio too smart and therefore letting EnergyPlus report certain errors. I wouldn't be opposed to just accepting the generic Curve type throught OpenStudio and letting EnergyPlus report if there are errors.

That said if we do go with validation in OpenStudio then option 2, but with a new ::setCurve method seems right. Paying the performance penality for every call to the existing ::setPointer seems unacceptable to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm opening an issue here, that we may just end up closing. I realize the Curve objects are already fine (via the \reference check), only the TableLookup has this issue:

@jmarrec jmarrec marked this pull request as ready for review September 26, 2023 11:24
@jmarrec jmarrec merged commit 950172b into develop Sep 26, 2023
4 of 6 checks passed
@jmarrec jmarrec deleted the table-lookup-fixes branch September 26, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIChange A motivated non-backward compatible change that breaks the existing API and needs to be communicated component - Model Enhancement Request Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WaterToAirHeatPumpEquationFit curves do not support Table:Lookup
4 participants