Fixes `Output:JSON` tabular data output incorrectly dependent on `OutputControl:Files input` #10155

shorowit · 2023-08-19T01:18:38Z

Pull request overview

Fixes Output:JSON tabular data output incorrectly dependent on OutputControl:Files input #9393
Allows tabular data in JSON output files without also needing to create the tabular output file itself.

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

Pull Request Author

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

Reviewer

This will not be exhaustively relevant to every PR.

… tabular output file.

src/EnergyPlus/EconomicTariff.cc

shorowit · 2023-08-19T01:27:36Z

To test, add an Output:JSON object to any test file, like so:

Output:JSON,
  TimeSeriesAndTabular,                   !- Option Type
  Yes;                                    !- Output JSON

Note that you now get the same eplusout.json output whether you have:

OutputControl:Files,
  ,                                    !- Output CSV
  ,                                    !- Output MTR
  ,                                    !- Output ESO
  ,                                    !- Output EIO
  Yes;                                 !- Output Tabular

or

OutputControl:Files,
  ,                                    !- Output CSV
  ,                                    !- Output MTR
  ,                                    !- Output ESO
  ,                                    !- Output EIO
  No;                                  !- Output Tabular

JasonGlazer · 2023-09-01T19:54:47Z

@shorowit I think the lines:

if (!(state.files.outputControl.tabular || state.files.outputControl.sqlite || state.files.outputControl.json))

should probably be something like

if (!(state.files.outputControl.tabular || state.files.outputControl.sqlite || (state.files.outputControl.json & state.dataResultsFramework->resultsFramework->timeSeriesAndTabularEnabled()) ))

Honestly, the portion that says just says "state.files.outputControl.sqlite" doesn't seem quite correct to me for the same reason but I haven't tried testing that.

Myoldmopar · 2023-09-01T20:03:40Z

And if we go that way, I would prefer a valid version of something like this:

bool const htmlTabular = state.files.outputControl.tabular;
bool const jsonTabular = state.files.outputControl.json && state.dataResultsFramework->resultsFramework->timeSeriesAndTabularEnabled();
bool const sqliteTabular = state.files.outputControl.sqlite; // && @JasonGlazer thinks something else maybe?
if (!(htmlTabular || jsonTabular || sqliteTabular))

…put-json-tabular-data

shorowit · 2023-09-01T21:09:51Z

@Myoldmopar @JasonGlazer Is this what you had in mind? It seems weird to me that the code in four different places is setting state.dataOutRptTab->WriteTabularFiles = false;, but maybe that's fine. I'm not that familiar with what is going on.

JasonGlazer · 2023-09-01T21:23:19Z

@shorowit that seems like a nice approach keeping the logic in the function. I didn't mean to imply that I know that my suggestion will fix the problem that @Myoldmopar brought up but I think it will. I would recommend either running some test files to make sure it works as expected and then adding some unit tests. Some digging might be needed if the SQL is not working right.

src/EnergyPlus/IOFiles.cc

shorowit · 2023-09-14T19:28:10Z

I don't have an immediate need for this improvement, and I'm not going to have a chance to further test this and bring it to completion in the near future. So I'm going to change the milestone to the next release. (If someone else wants to bring it to completion sooner, feel free to have at it.)

nrel-bot-3 · 2023-10-13T00:09:51Z

@shorowit @Myoldmopar it has been 28 days since this pull request was last updated.

Myoldmopar · 2023-11-21T17:51:32Z

src/EnergyPlus/EconomicTariff.cc

@@ -2711,7 +2711,7 @@ void ComputeTariff(EnergyPlusData &state)

    Real64 annualAggregate;

-    if (!(state.files.outputControl.tabular || state.files.outputControl.sqlite)) {
+    if (!state.files.outputControl.writeTabular(state)) {


Dumb question here...why is the state.dataOutRptTab->WriteTabularFiles being set here in a function called ComputeTariff inside EconomicTariff.cc anyway?? That alone seems super weird.

It is a good question. And I don't remember, I suppose this might be the first place that a call to something dependent on actually writing tabular reports occurs, although that does not seem likely.

Myoldmopar · 2023-11-21T17:53:45Z

src/EnergyPlus/OutputReportTabular.cc

@@ -323,7 +323,7 @@ void GetInputTabularMonthly(EnergyPlusData &state)
    // SUBROUTINE LOCAL VARIABLE DECLARATIONS:
    auto &ort = state.dataOutRptTab;

-    if (!(state.files.outputControl.tabular || state.files.outputControl.sqlite)) {
+    if (!state.files.outputControl.writeTabular(state)) {


I think I'd like to take a crack at this where state.dataOutRptTab->WriteTabularFiles is set once very early in the simulation and we don't need to keep calling this function. As soon as we have read inputs, we should be able to assign that flag.

I like that the function collects all the flags in one place -- that's always welcomed. The flag checks themselves may need a little extra work as described above, but regardless it's a good step.

Sounds good.

JasonGlazer

I have not tested it to make sure the files are produced as expected but the logic looks good to me. Thanks @shorowit and @Myoldmopar

JasonGlazer · 2023-11-27T16:20:02Z

src/EnergyPlus/EconomicTariff.cc

@@ -2711,7 +2711,7 @@ void ComputeTariff(EnergyPlusData &state)

    Real64 annualAggregate;

-    if (!(state.files.outputControl.tabular || state.files.outputControl.sqlite)) {
+    if (!state.files.outputControl.writeTabular(state)) {


It is a good question. And I don't remember, I suppose this might be the first place that a call to something dependent on actually writing tabular reports occurs, although that does not seem likely.

JasonGlazer · 2023-11-27T16:56:20Z

src/EnergyPlus/IOFiles.cc

+    bool const sqliteTabular = state.files.outputControl.sqlite; // && @JasonGlazer thinks something else maybe?
+    return (htmlTabular || jsonTabular || sqliteTabular);
+}
+


The only reason I was concerned is the Output:SQLite field "Option Type" has two options Simple, SimpleAndTabular and they don't seem to impact this.

JasonGlazer · 2023-11-27T16:56:56Z

src/EnergyPlus/OutputReportTabular.cc

@@ -323,7 +323,7 @@ void GetInputTabularMonthly(EnergyPlusData &state)
    // SUBROUTINE LOCAL VARIABLE DECLARATIONS:
    auto &ort = state.dataOutRptTab;

-    if (!(state.files.outputControl.tabular || state.files.outputControl.sqlite)) {
+    if (!state.files.outputControl.writeTabular(state)) {


Sounds good.

nrel-bot-2 · 2023-12-26T00:08:25Z

@Myoldmopar @shorowit @Myoldmopar it has been 28 days since this pull request was last updated.

Myoldmopar · 2024-01-05T14:59:48Z

OK, this has lingered long enough. It seems good. I'm going to just pull and test and get this in. If some further improvements are needed, they can come later.

Myoldmopar · 2024-01-05T15:06:27Z

Everything is running fine, merging this. Thanks @shorowit and @JasonGlazer.

Allows tabular data in Output:JSON without also needing to create the…

2842ce0

… tabular output file.

shorowit added the Defect Includes code to repair a defect in EnergyPlus label Aug 19, 2023

shorowit added this to the EnergyPlus 23.2 milestone Aug 19, 2023

shorowit self-assigned this Aug 19, 2023

shorowit commented Aug 19, 2023

View reviewed changes

src/EnergyPlus/EconomicTariff.cc Outdated Show resolved Hide resolved

JasonGlazer self-requested a review August 30, 2023 19:12

shorowit added 2 commits September 1, 2023 14:19

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into out…

411c198

…put-json-tabular-data

Address comments.

51a6a08

rraustad reviewed Sep 1, 2023

View reviewed changes

src/EnergyPlus/IOFiles.cc Show resolved Hide resolved

shorowit modified the milestones: EnergyPlus 23.2, EnergyPlus 24.1 Sep 14, 2023

Myoldmopar assigned Myoldmopar and unassigned shorowit Oct 25, 2023

Myoldmopar reviewed Nov 21, 2023

View reviewed changes

JasonGlazer approved these changes Nov 27, 2023

View reviewed changes

Myoldmopar merged commit bc827aa into develop Jan 5, 2024
10 of 15 checks passed

Myoldmopar deleted the output-json-tabular-data branch January 5, 2024 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes `Output:JSON` tabular data output incorrectly dependent on `OutputControl:Files input` #10155

Fixes `Output:JSON` tabular data output incorrectly dependent on `OutputControl:Files input` #10155

shorowit commented Aug 19, 2023 •

edited

shorowit commented Aug 19, 2023

JasonGlazer commented Sep 1, 2023

Myoldmopar commented Sep 1, 2023

shorowit commented Sep 1, 2023

JasonGlazer commented Sep 1, 2023

shorowit commented Sep 14, 2023

nrel-bot-3 commented Oct 13, 2023

Myoldmopar Nov 21, 2023

JasonGlazer Nov 27, 2023

Myoldmopar Nov 21, 2023

JasonGlazer Nov 27, 2023

JasonGlazer left a comment

JasonGlazer Nov 27, 2023

JasonGlazer Nov 27, 2023

JasonGlazer Nov 27, 2023

nrel-bot-2 commented Dec 26, 2023

Myoldmopar commented Jan 5, 2024

Myoldmopar commented Jan 5, 2024

Fixes Output:JSON tabular data output incorrectly dependent on OutputControl:Files input #10155

Fixes Output:JSON tabular data output incorrectly dependent on OutputControl:Files input #10155

Conversation

shorowit commented Aug 19, 2023 • edited

Pull request overview

Pull Request Author

Reviewer

shorowit commented Aug 19, 2023

JasonGlazer commented Sep 1, 2023

Myoldmopar commented Sep 1, 2023

shorowit commented Sep 1, 2023

JasonGlazer commented Sep 1, 2023

shorowit commented Sep 14, 2023

nrel-bot-3 commented Oct 13, 2023

Myoldmopar Nov 21, 2023

Choose a reason for hiding this comment

JasonGlazer Nov 27, 2023

Choose a reason for hiding this comment

Myoldmopar Nov 21, 2023

Choose a reason for hiding this comment

JasonGlazer Nov 27, 2023

Choose a reason for hiding this comment

JasonGlazer left a comment

Choose a reason for hiding this comment

JasonGlazer Nov 27, 2023

Choose a reason for hiding this comment

JasonGlazer Nov 27, 2023

Choose a reason for hiding this comment

JasonGlazer Nov 27, 2023

Choose a reason for hiding this comment

nrel-bot-2 commented Dec 26, 2023

Myoldmopar commented Jan 5, 2024

Myoldmopar commented Jan 5, 2024

Fixes `Output:JSON` tabular data output incorrectly dependent on `OutputControl:Files input` #10155

Fixes `Output:JSON` tabular data output incorrectly dependent on `OutputControl:Files input` #10155

shorowit commented Aug 19, 2023 •

edited