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 #4647, wrap the E+ Table:Lookup, Table:IndependentVariableList, and Table:IndependentVariable objects #4652

Merged
merged 66 commits into from Sep 8, 2022

Conversation

joseph-robertson
Copy link
Collaborator

@joseph-robertson joseph-robertson commented Aug 19, 2022

Pull request overview

Possible implementation:

  • Table:Lookup (Curve?)
  • Table:IndependentVariableList (Use ModelObjectList?)
  • Table:IndependentVariable (ParentObject?)

much like the E+ IDD:

  Table:IndependentVariable,
    HPACCoolCapFFF_IndependentVariable1,  !- Name
    Cubic,                   !- Interpolation Method
    Constant,                !- Extrapolation Method
    0.5,                     !- Minimum Value
    1.5,                     !- Maximum Value
    ,                        !- Normalization Reference Value
    Dimensionless,           !- Unit Type
    ,                        !- External File Name
    ,                        !- External File Column Number
    ,                        !- External File Starting Row Number
    0.0,                     !- Value 1
    1.0,                     !- Value 2
    1.5;                     !- <none>

  Table:IndependentVariableList,
    HPACCoolCapFFF_IndependentVariableList,  !- Name
    HPACCoolCapFFF_IndependentVariable1;  !- Independent Variable 1 Name

  Table:Lookup,
    HPACCoolCapFFF,          !- Name
    HPACCoolCapFFF_IndependentVariableList,  !- Independent Variable List Name
    ,                        !- Normalization Method
    ,                        !- Normalization Divisor
    0.8,                     !- Minimum Output
    1.5,                     !- Maximum Output
    Dimensionless,           !- Output Unit Type
    ,                        !- External File Name
    ,                        !- External File Column Number
    ,                        !- External File Starting Row Number
    0.8,                     !- Output Value 1
    1.0,                     !- Output Value 2
    1.1;                     !- <none>

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. labels Aug 19, 2022
@joseph-robertson joseph-robertson self-assigned this Aug 19, 2022
@joseph-robertson
Copy link
Collaborator Author

@jmarrec Would you be able to do a cursory review of the general approach here? i.e., creating classes of TableLookup and TableIndependentVariable and then using ModelObjectLists to store the list of independent variables, etc?

@jmarrec
Copy link
Collaborator

jmarrec commented Aug 22, 2022

@joseph-robertson, some thoughts:

Possible implementation:

* `Table:Lookup` (Curve?)
  • Yeah, this one is a Curve (< ResourceObject < ParentObject < ModelObject)
* `Table:IndependentVariableList` (Use `ModelObjectList`?)
  • ModelObjectList would work yes. But its behavior relating to clone/remove might be annoying (I believe it'll clone the underlying objects, regardless of whether they are ResourceObjects or not, so you might have to work around that... make sure to write extensive tests for that)
* `Table:IndependentVariable` (ParentObject?)
  • I would say ResourceObject actually. It could technically be shared by several TableLookup objects.
    • Eg: you have a CSV file (or you use the extensible fields instead of using an external file, doesn't matter conceptually) where the first column is the temperature (independent variable), and the second and third columns are the EIRFT and CAPFT values (dependent variables/output), then you would have two TableLookUp using the same Table:IndependentVariable object.

Comment on lines +77 to +81
// Value
for (const double& value : modelObject.values()) {
IdfExtensibleGroup eg = idfObject.pushExtensibleGroup();
eg.setDouble(Table_IndependentVariableExtensibleFields::Value, value);
}
Copy link
Collaborator

@jmarrec jmarrec Aug 31, 2022

Choose a reason for hiding this comment

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

  • I think we should enforce ascending order here (?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jmarrec Is this still a TODO?

…he model conversion in ForwardTransaltor.cpp)) + Macroize the ignore of deprecated methods (DISABLE_WARNING_DEPRECATED) for clarity/shortness

change macro formatting
Copy link
Collaborator

@jmarrec jmarrec left a comment

Choose a reason for hiding this comment

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

@joseph-robertson I added a ReverseTranslator. I opened a PR to NREL/OpenStudio-resources#173 which caught a few bugs and I added an explicit test for sharing two TableIndependentVariables on two TableLookups.

src/energyplus/ReverseTranslator.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Wrap the E+ Table:Lookup, Table:IndependentVariableList, and Table:IndependentVariable objects.
3 participants