Addresses #3784, add methods to edit SummaryReports in OpenStudio SDK #3882

joseph-robertson · 2020-02-18T22:04:27Z

Pull request overview

Fixes Add methods to edit SummaryReports in OpenStudio SDK #3784
Instead of always requesting "AllSummary", now you can individually request summary reports of interest (or no reports at all)
This PR needs to wait on a fix for cloning unique model objects

Please read OpenStudio Pull Requests to better understand the OpenStudio Pull Request protocol.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

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

kbenne · 2020-02-24T15:59:26Z

@DavidGoldwasser and @joseph-robertson is there any possibility that this feature could accidentally break the reporting measure? Could a user turn off certain summary reports that the reporting measure might need?

joseph-robertson · 2020-02-24T19:27:06Z

@kbenne That's a good question. What's the name of the reporting measure, and where can it be found?

jmarrec · 2020-02-25T10:14:31Z

@joseph-robertson https://github.com/NREL/OpenStudio-measures/tree/develop/nrel_published/openstudio_results

Probably a good idea to run the entire test-suite on nrel_published in fact.

jmarrec

Thorough PR @joseph-robertson!

Most of my review comments are really minor things.

The real comment is to adjust the default behavior of adding "AllSummary" (I think user should be able to disable all reports), in addition to testing with the reporting measure.

This should receive an OpenStudio-resources tests, along with the OutputDiagnostics etc: added it in issue NREL/OpenStudio-resources#94

jmarrec · 2020-02-25T10:19:55Z

src/energyplus/ForwardTranslator.cpp

-    IdfObject outputTableSummaryReport(IddObjectType::Output_Table_SummaryReports);
-    IdfExtensibleGroup eg = outputTableSummaryReport.pushExtensibleGroup();
-    eg.setString(Output_Table_SummaryReportsExtensibleFields::ReportName,"AllSummary");
-    m_idfObjects.push_back(outputTableSummaryReport);


This will break backward compatibility in its current form. cf comment on line 558

jmarrec · 2020-02-25T10:21:28Z

src/energyplus/ForwardTranslator.cpp

+    // ensure that output table summary reports exists
+    boost::optional<model::OutputTableSummaryReports> summaryReports = model.getOptionalUniqueModelObject<model::OutputTableSummaryReports>();
+    if (!summaryReports){
+      summaryReports = model.getUniqueModelObject<model::OutputTableSummaryReports>();


To maintain backward compat, either OutputTableSummaryReports needs to have it's Ctor directly call addSummaryReport("AllSummary") or you need to do it here by adding a line after 561 summaryReports.addSummaryReport("AllSummary");

I now see you actually do that in the FT translateOutputTableSumaryReports method, but I like that less, see comments over there too.

jmarrec · 2020-02-25T10:23:54Z

src/energyplus/ForwardTranslator/ForwardTranslateOutputTableSummaryReports.cpp

+    }
+  } else {
+    auto eg = idfObject.pushExtensibleGroup();
+    eg.setString(Output_Table_SummaryReportsExtensibleFields::ReportName, "AllSummary");


I see you actually handle the case where we create a default one in FT here. I'd prefer doing it in ForwardTranslator.cpp#L558 (or model API), so the user can choose to manually instantiate an OutputTableSummaryReports and actually disable all outputs instead of ending with "AllSummary" anyways.

jmarrec · 2020-02-25T10:26:42Z

src/energyplus/ReverseTranslator/ReverseTranslateOutputTableSummaryReports.cpp

+    LOG(Error, "WorkspaceObject is not IddObjectType: OutputTableSummaryReports");
+    return boost::none;
+  }
+


Perhaps worth checking if it actually has reports, if not return and don't create one? (minor comment...)

src/energyplus/Test/OutputTableSummaryReports_GTest.cpp

src/model/test/OutputTableSummaryReports_GTest.cpp

jmarrec · 2020-02-25T10:53:01Z

src/model/test/OutputTableSummaryReports_GTest.cpp

+  // change some of the fields
+  outputTableSummaryReports.addSummaryReport("ClimaticDataSummary");
+
+  // clone it into the same model


Cloning a Unique model object?

jmarrec · 2020-02-25T10:53:54Z

src/model/test/OutputTableSummaryReports_GTest.cpp

+  // clone it into a different model
+  Model model2;
+  OutputTableSummaryReports outputTableSummaryReportsClone2 = outputTableSummaryReports.clone(model2).cast<OutputTableSummaryReports>();
+  ASSERT_EQ(1, outputTableSummaryReportsClone2.numberofSummaryReports());


This is a unique model object. What happens if model2 already has one instantiated? (I never thought of that...)

Also, probably test if the report is "ClimaticDataSummary"

src/model/OutputTableSummaryReports_Impl.hpp

jmarrec · 2020-02-25T11:29:07Z

@joseph-robertson I made a few trivial changes and "resolved" my review comments above accordingly to clean up the review and focus on the important stuff. I also added two synonym methods validReportNameValues and reportNameValues to return the \choices. (perhaps you'll want to change the name to summaryReportValues to match the API naming you chose, I just used the E+ IDD field name like in most cases)

joseph-robertson · 2020-02-25T23:32:15Z

This PR needs to wait on a fix to cloning unique model objects.

@jmarrec @kbenne

…n required)

(Adding tests for all these objects in NREL/OpenStudio-resources#94 at the same time)

…ated

joseph-robertson added 10 commits February 18, 2020 13:26

Update openstudio idd.

f9d7574

Stub source code files.

81fdba4

Stub ft files.

5ce51a1

Stub rt files.

6021845

Stub model tests file.

fee04e2

Update cmakelists files.

af543b9

Update other files.

d94f051

Update the idd.

e166e6a

Fill in source code and test files.

455f3b0

Fill in ft, rt, and test files.

1a4f5ff

joseph-robertson added Enhancement Request component - Model labels Feb 18, 2020

joseph-robertson added this to the OpenStudio SDK 3.0.0 milestone Feb 18, 2020

joseph-robertson self-assigned this Feb 18, 2020

joseph-robertson added 4 commits February 18, 2020 15:07

Bit of cleanup to model cpp.

ede1efd

Add tests for ft.

f83fea6

Update ft and rt files.

6e17834

Update model files.

80b714d

joseph-robertson added the Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. label Feb 19, 2020

joseph-robertson marked this pull request as ready for review February 19, 2020 23:20

joseph-robertson added 2 commits February 20, 2020 09:47

Remove pushed ext group if empty.

ea973b8

Add a few more model tests.

0eec636

joseph-robertson requested a review from jmarrec February 20, 2020 16:48

Merge branch 'develop' into issue-3784

36efda7

jmarrec mentioned this pull request Feb 25, 2020

Add test for OutputDiagnostics, OutputDebuggingData, OutputJSON and OutputTableSummaryReports NREL/OpenStudio-resources#94

Closed

jmarrec reviewed Feb 25, 2020

View reviewed changes

src/model/OutputTableSummaryReports_Impl.hpp Outdated Show resolved Hide resolved

jmarrec added 4 commits February 25, 2020 12:16

Lint and use bool removeSummaryReport(index) instead of void

9ea90c6

Pass string and vector<string> by const-reference

c4dcf36

Add a method to return valid choices for ReportNames

d857776

Comments in tests and use "EXPECT_" instead of "ASSERT_" when possible

6ad4821

Bad name for validReportNameValues

7c13872

jmarrec mentioned this pull request Feb 25, 2020

Unique ModelObjects shouldn't be allowed to be cloned into the same model and cloning into another model should be handled #3888

Closed

jmarrec and others added 5 commits February 25, 2020 14:28

summaryReportIndex should be case-insensitive

fccb780

Add a convenience function to enable "AllSummary"

4dc314c

Update model source and tests.

f414979

Update ft, rt, and tests.

22a0363

Some more cleanup.

83ae3b2

joseph-robertson added the severity - Normal Bug label Feb 25, 2020

jmarrec added 3 commits February 26, 2020 10:37

Adjust tests to use EXPECT_ everywhere possible (and ASSERT_ only whe…

48ac925

…n required)

DOn't FT if reports are empty

8d18c44

Unrelated minor change as a follow up for #3875

1b4c8fa

(Adding tests for all these objects in NREL/OpenStudio-resources#94 at the same time)

jmarrec mentioned this pull request Feb 26, 2020

#94 - Add test for OutputDiagnostics, OutputDebuggingData, OutputJSON, and OutputTableSummaryReports NREL/OpenStudio-resources#96

Merged

23 tasks

jmarrec and others added 2 commits February 26, 2020 14:56

Update FT test where we have the object, with 0 reports => Not transl…

bee72a5

…ated

Merge branch 'develop' into issue-3784

d701d07

tijcolem mentioned this pull request Mar 12, 2020

Merge city final #3915

Merged

tijcolem closed this in #3915 Mar 12, 2020

joseph-robertson deleted the issue-3784 branch March 16, 2020 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addresses #3784, add methods to edit SummaryReports in OpenStudio SDK #3882

Addresses #3784, add methods to edit SummaryReports in OpenStudio SDK #3882

joseph-robertson commented Feb 18, 2020 •

edited

Loading

kbenne commented Feb 24, 2020

joseph-robertson commented Feb 24, 2020

jmarrec commented Feb 25, 2020

jmarrec left a comment •

edited

Loading

jmarrec Feb 25, 2020

jmarrec Feb 25, 2020 •

edited

Loading

jmarrec Feb 25, 2020

jmarrec Feb 25, 2020

jmarrec Feb 25, 2020

jmarrec Feb 25, 2020

jmarrec Feb 25, 2020

jmarrec commented Feb 25, 2020 •

edited

Loading

joseph-robertson commented Feb 25, 2020

Addresses #3784, add methods to edit SummaryReports in OpenStudio SDK #3882

Addresses #3784, add methods to edit SummaryReports in OpenStudio SDK #3882

Conversation

joseph-robertson commented Feb 18, 2020 • edited Loading

Pull request overview

Pull Request Author

Review Checklist

kbenne commented Feb 24, 2020

joseph-robertson commented Feb 24, 2020

jmarrec commented Feb 25, 2020

jmarrec left a comment • edited Loading

Choose a reason for hiding this comment

jmarrec Feb 25, 2020

Choose a reason for hiding this comment

jmarrec Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

jmarrec Feb 25, 2020

Choose a reason for hiding this comment

jmarrec Feb 25, 2020

Choose a reason for hiding this comment

jmarrec Feb 25, 2020

Choose a reason for hiding this comment

jmarrec Feb 25, 2020

Choose a reason for hiding this comment

jmarrec Feb 25, 2020

Choose a reason for hiding this comment

jmarrec commented Feb 25, 2020 • edited Loading

joseph-robertson commented Feb 25, 2020

joseph-robertson commented Feb 18, 2020 •

edited

Loading

jmarrec left a comment •

edited

Loading

jmarrec Feb 25, 2020 •

edited

Loading

jmarrec commented Feb 25, 2020 •

edited

Loading