#8249 - Adjustments for OutputControl:Files: properly register it, and do not output END file if set to No

[jmarrec]

Pull request overview

• Fix OutputControl:Files isn't processed correctly. #8249
• Trivial Adjustments for OutputControl:Files: properly register it, and do not output END file if set to No

Pull Request Author

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

• 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
• If any defect files are updated to a more recent version, upload new versions here or on DevSupport
• If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
• If structural output changes, add to output rules file and add OutputChange label
• If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

• 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

[mjwitte]

@jmarrec This is good for the most part. The object now works as expected to enable/disable various outputs.

One side effect is that EP-Launch (classic on windows) declares a crash if eplusout.end is not present. @JasonGlazer does EP-Launch3 use the same approach to detect a successful run?

The direct csv outputs have an extra CR character (OD OD OA on windows) on the header line.

The direct csv for output variables works for 5ZoneAirCooled and matches the csv produced using ReadVarsESO (with no rvi) except for the small detail that ReadVarsESO adds a trailing space to every line which makes no difference.

But the direct csv for meter variables is messed up. It's monthly output, so it may be an issue with monthly reporting rather than the meter specifically. Tested hourly meter - ok, and monthly output variables - ok. So it's just a problem with monthly meters (need to test all reporting frequencies).

I'll post a new issue for the direct csv output problems, but we need to decide if we want to disable setting Output END = No for now?

[JasonGlazer]

@mjwitte yes, I believe EP-Launch3 also reads the .end file to see if the run was successful. I'm not sure why we would want to turn off the creation of such a lightweight file that is being used by by EP-Launch and EP-Launch3 and probably other software to determine if the simulation was successful, had errors, or crashed.

[mbadams5]

@mjwitte, shouldn't this currently output the END file by default? My take is that if someone is explicitly turning off the file, they are accepting the potential consequences of that action, including EP-Launch not working correctly. There are instances where you may not want to write any extraneous outputs files, even if small.

[mjwitte]

Understood, but the average users won't know that. So, we either need to add something to the ep-launch crash message and/or add something to the documentation and an idd note warning of the side effect.

[mbadams5]

I am fine with either approach.

[Myoldmopar]

One thing about EPLaunch3 is that the end file handling is handled in our workflows that we package up with EnergyPlus. In those workflows, we already handle an error return code from E+ to avoid trying to open up any output files if the simulation failed. However, in the case of a valid simulation, we do naively pop open the output file to parse out the errors/runtime/etc. If we wanted to allow missing end files, we should probably go ahead and tweak that workflow just a smidge. We could just return 'unknown' for the runtime/errors.

[mjwitte]

@mbadams5 Do the code changes here look good to you? If so, this should probably drop in as-is and the issues in #8252 can be dealt with separately.

[mbadams5]

The changes look good except for my one comment, which should be addressed.

[mbadams5]

I went ahead and quickly tested it. It ignores the END file when specified with my changes.

[mitchute]

I verified this fix. Anything else we need to do here?

Also, I'm not seeing any documentation or example files for this new feature. Should they be added before the release?

[mjwitte]

Also, I'm not seeing any documentation or example files for this new feature. Should they be added before the release?

@mitchute Yes, either here or in another PR to fix #8252.

[mitchute]

OK, I'll merge this one and start taking a look at the other issue referenced. We can deal with these remaining loose ends over there.