Skip to content

Introduce optional sort option field for output control#11523

Draft
joseph-robertson wants to merge 5 commits intodevelopfrom
tables-sorted
Draft

Introduce optional sort option field for output control#11523
joseph-robertson wants to merge 5 commits intodevelopfrom
tables-sorted

Conversation

@joseph-robertson
Copy link
Copy Markdown
Collaborator

@joseph-robertson joseph-robertson commented Apr 13, 2026

Pull request overview

Description of the purpose of this PR

Pull Request Author

  • 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
  • If adding/removing any output files (e.g., eplustbl.*)
    • Update ..\scripts\Epl-run.bat
    • Update ..\scripts\RunEPlus.bat
    • Update ..\src\EPLaunch\ MainModule.bas, epl-ui.frm, and epl.vbp (VersionComments)
    • Update ...github\workflows\energyplus.py

Reviewer

  • 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

@joseph-robertson joseph-robertson self-assigned this Apr 13, 2026
@joseph-robertson joseph-robertson added NewFeature Includes code to add a new feature to EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels Apr 13, 2026
@joseph-robertson joseph-robertson mentioned this pull request Apr 13, 2026
Closed
25 tasks
joseph-robertson
joseph-robertson commented Apr 13, 2026
View reviewed changes
Comment thread idd/Energy+.idd.in
\key Yes
\key No
\default Yes
A4; \field Sort Option
Copy link
Copy Markdown
Collaborator Author

@joseph-robertson joseph-robertson Apr 13, 2026

Choose a reason for hiding this comment

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

Field name and keys borrowed from Output:VariableDictionary.

Comment thread src/EnergyPlus/OutputReportTabular.cc Outdated
int colsBody)
{
int index = 0;
auto it = std::find(std::begin(columnLabels), std::end(columnLabels), "Name");
Copy link
Copy Markdown
Collaborator Author

@joseph-robertson joseph-robertson Apr 13, 2026

Choose a reason for hiding this comment

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

There are probably other column names we'd want to sort on (and not just "Name"). Not sure how to handle this.

Copy link
Copy Markdown
Contributor

@JasonGlazer JasonGlazer Apr 14, 2026

Choose a reason for hiding this comment

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

Many times, the first column that you would want to sort on doesn't have a column heading at all (almost all predefined tables).

Monthly tables probably should not be sorted.

Tables that have statistics (sum, average, min, or max) as the last rows should probably keep them as the last rows and not intermingle them (like monthly tables).

The Initialization Summary has all the tables with a row count in the first column, so that they are in the same order as the EIO file, but sometimes the second column is the zone name or the object name, so maybe they should be sorted by the second column.

Overall, this is a good idea, but complicated with a lot of cases to consider.

Maybe the approach is that for each predefined table, a column that would make sense to sort on is optionally defined. If it is not defined, then maybe just the first column is used.

Copy link
Copy Markdown
Collaborator Author

@joseph-robertson joseph-robertson Apr 14, 2026

Choose a reason for hiding this comment

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

Thanks @JasonGlazer, this is very useful information. And agreed that this gets complicated with a lot of cases to consider.

Comment thread src/EnergyPlus/OutputReportTabular.cc Outdated
}

if (currentStyle.produceTabular) {
if (tableName == "ScheduleTypeLimits") {
Copy link
Copy Markdown
Collaborator Author

@joseph-robertson joseph-robertson Apr 13, 2026

Choose a reason for hiding this comment

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

(Just testing on this table for right now.)

Comment thread src/EnergyPlus/OutputReportTabular.cc
}
WriteSubtitle(state, tableName);
std::string footnote;
WriteTable(state, tableBody, rowHead, columnHead, columnWidth, false, footnote);
Copy link
Copy Markdown
Collaborator Author

@joseph-robertson joseph-robertson Apr 13, 2026

Choose a reason for hiding this comment

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

We could probably move the sort to within WriteTable, but putting it here may give more control (the table name isn't passed into WriteTable).

Comment thread testfiles/Supermarket.idf
HTML, !- Column Separator
, !- Unit Conversion
, !- Format Numeric Values
Name; !- Sort Option
Copy link
Copy Markdown
Collaborator Author

@joseph-robertson joseph-robertson Apr 13, 2026

Choose a reason for hiding this comment

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

We could introduce similar field for Output:JSON and Output:SQLite? I'm not totally sure how these three objects are all related.

Copy link
Copy Markdown
Contributor

@JasonGlazer JasonGlazer Apr 14, 2026

Choose a reason for hiding this comment

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

For Output:JSON it would be good to have something similar. I'm not so sure if SQLite.

@joseph-robertson joseph-robertson added the DoNotMerge Code that requires additional attention and investigation label Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JasonGlazer JasonGlazer JasonGlazer left review comments

Assignees

@joseph-robertson joseph-robertson

Labels

DoNotMerge Code that requires additional attention and investigation IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) NewFeature Includes code to add a new feature to EnergyPlus

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

idf converted to epjson alphabetizes by object name

3 participants

@joseph-robertson @JasonGlazer @nrel-bot-2c