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

Allow regular expressions as keys for Output:Variable #6017

Merged
merged 29 commits into from Feb 16, 2017

Conversation

Projects
None yet
9 participants
@mbadams5
Copy link
Contributor

commented Feb 16, 2017

Pull request overview

This pull request allows regular expressions to be used to better filter Output:Variable keys. Previously, only the exact key or all keys (*) were valid options. Now most typical regular expressions are allowed as options. Check RE2 Syntax for allowed regular expressions as well as what regular expression syntax RE2 supports.

One key note is that ANY regular expression with a comma (,) will fail regardless if it is valid regular expression syntax or not. This is due to the nature of the EnergyPlus input processor since fields are delimited by commas. Thus any comma is seen as the end of one field and the beginning of another.

How this regular expression filtering is powerful:

Given the following IDF snippet

NodeList, Zone1Inlets, SalesFloor InletNode 1;
NodeList, Zone2Inlets, SalesFloor InletNode 2;
NodeList, Zone3Inlets, SalesFloor InletNode 3;
NodeList, Zone4Inlets, Office InletNode 1;
NodeList, Zone5Inlets, Office InletNode 2;
NodeList, Zone6Inlets, Office InletNode 3;

Previously, if you wanted all SalesFloor InletNode's System Node Mass Flow Rate, you had to individually write each as an Output:Variable as follows...

Output:Variable, SalesFloor InletNode 1, System Node Mass Flow Rate, timestep;
Output:Variable, SalesFloor InletNode 2, System Node Mass Flow Rate, timestep;
Output:Variable, SalesFloor InletNode 3, System Node Mass Flow Rate, timestep;

Or you could select ALL nodes System Node Mass Flow Rate, which would include the Office nodes...

Output:Variable, *, System Node Mass Flow Rate, timestep;

Now you can write it much more concisely with a regular expression...

Output:Variable, SalesFloor InletNode [\d]+, System Node Mass Flow Rate, timestep;

This is just a simple example, but begins to show the filtering capabilities.

Work Checklist

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)
  • At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
    • Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
    • Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
    • NewFeature: This pull request includes code to add a new feature to EnergyPlus
    • Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
    • DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

  • Code style (parentheses padding, variable names)
  • Functional code review (it has to work!)
  • If defect, results of running current develop vs this branch should exhibit the fix
  • CI status: all green or justified
  • Performance: CI Linux results include performance check -- verify this
  • Unit Test(s)
  • C++ checks:
    • Argument types
    • If any virtual classes, ensure virtual destructor included, other things
  • IDD changes:
    • Verify naming conventions and styles, memos and notes and defaults
    • Open windows IDF Editor with modified IDD to check for errors
    • If transition, add rules to spreadsheet
    • If transition, add transition source
    • If transition, update idfs
  • If new idf included, locally check the err file and other outputs
  • Documentation changes in place
  • Changed docs build successfully
  • ExpandObjects changes?
  • If output changes, including tabular output structure, add to output rules file for interfaces
@nrel-bot-3

This comment has been minimized.

Copy link

commented on 96e6c3b Feb 9, 2017

output_variable_regex (mbadams5) - x86_64-MacOS-10.9-clang: OK (2315 of 2373 tests passed, 5 test warnings)

  • 1 test had: ERR diffs.
  • 4 tests had: AUD diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Feb 11, 2017

output_variable_regex (mbadams5) - x86_64-Linux-Ubuntu-14.04-gcc-4.8: Tests Failed (595 of 2413 tests passed, 0 test warnings)

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Feb 11, 2017

output_variable_regex (mbadams5) - x86_64-Linux-Ubuntu-14.04-cppcheck-1.61: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Feb 11, 2017

output_variable_regex (mbadams5) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-UnitTestsCoverage-Debug: OK (1221 of 1221 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Feb 11, 2017

output_variable_regex (mbadams5) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-IntegrationCoverage-Debug: OK (1801 of 1801 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Feb 11, 2017

output_variable_regex (mbadams5) - x86_64-Linux-Ubuntu-14.04-custom_check: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Feb 12, 2017

output_variable_regex (mbadams5) - i386-Windows-7-VisualStudio-14: OK (2379 of 2379 tests passed, 0 test warnings)

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Feb 12, 2017

output_variable_regex (mbadams5) - Win64-Windows-7-VisualStudio-14: OK (2379 of 2379 tests passed, 0 test warnings)

Build Badge Test Badge

@mbadams5 mbadams5 added the NewFeature label Feb 16, 2017

@mbadams5 mbadams5 added this to the EnergyPlus 8.7.0 milestone Feb 16, 2017

@mbadams5 mbadams5 requested a review from Myoldmopar Feb 16, 2017

@mbadams5 mbadams5 requested a review from mjwitte Feb 16, 2017

@mbadams5 mbadams5 added the IDDChange label Feb 16, 2017

@nrel-bot-3

This comment has been minimized.

Copy link

commented on 2a06952 Feb 16, 2017

output_variable_regex (mbadams5) - x86_64-MacOS-10.9-clang: OK (2404 of 2408 tests passed, 8 test warnings)

  • 2 tests had: ERR diffs.
  • 7 tests had: AUD diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Feb 17, 2017

output_variable_regex (mbadams5) - i386-Windows-7-VisualStudio-14: OK (0 of 0 tests passed, 0 test warnings)

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Feb 17, 2017

output_variable_regex (mbadams5) - Win64-Windows-7-VisualStudio-14: OK (0 of 0 tests passed, 0 test warnings)

Build Badge Test Badge

@Myoldmopar

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

The E+ changes are super minimal here, and look good. Unit tests look good. I think this can drop right in easily. This will be a big improvement! I'll give CI some time to get through the platforms.

@mjwitte

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

@mbadams5 Will this alter the case of the variable keys/names in the output?
And I assume the licensing of the 3rd party package has been cleared with @jasondegraw

@mjwitte

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

@mbadams5 Are there I/O Ref changes for this?

@jasondegraw

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

@mjwitte re2 is BSD-style, so we're all good.

@mbadams5

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2017

@mjwitte Yes, the \retaincase will alter the case of the variable keys in the output, at least as far as how I understand it to work. The issue was that without it, the input processor was uppercasing the key and was causing the regular expressions to not match. The code will still use a case-insensitive comparison fall back method if the regular expression engine doesn't find a match.

There are no needed changes to the existing I/O Ref but I guess I could add new text about the regular expressions if needed.

@Myoldmopar

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

Yeah, good call. The IO ref could definitely use some info. @mbadams5 Can you add some content to the relevant output object(s) to describe the capabilities and the limitations (no comma, repetition)?

mbadams5 added some commits Feb 16, 2017

Merge branch 'develop' into output_variable_regex
# Conflicts:
#	idd/Energy+.idd.in
Fix merge conflicts
# Conflicts:
#	idd/Energy+.idd.in
@mbadams5

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2017

@Myoldmopar I updated the I/O Ref to include information about the new regular expression option.

@nrel-bot-3

This comment has been minimized.

Copy link

commented on a703ef4 Feb 16, 2017

output_variable_regex (mbadams5) - x86_64-MacOS-10.9-clang: Tests Failed (2114 of 2408 tests passed, 60 test warnings)

  • 353 tests had: AUD diffs.
  • 351 tests had: ERR diffs.
  • 294 tests had: Table big diffs.
  • 3 tests had: Table small diffs.

Build Badge Test Badge

@Myoldmopar

This comment has been minimized.

Copy link
Member

commented Feb 16, 2017

OK, I'm going to get this in. If anything comes up due to the abrupt merge I'll deal with it. Thanks @mbadams5, this is a very cool enhancement.

@Myoldmopar Myoldmopar merged commit 46d0b39 into develop Feb 16, 2017

@Myoldmopar Myoldmopar deleted the output_variable_regex branch Feb 16, 2017

@mjwitte

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

@nrel-bot-3

This comment has been minimized.

Copy link

commented on ae978c0 Feb 16, 2017

output_variable_regex (mbadams5) - x86_64-MacOS-10.9-clang: OK (0 of 0 tests passed, 0 test warnings)

Build Badge Test Badge

@mbadams5 mbadams5 referenced this pull request Feb 17, 2017

Merged

Address issues due to Output:Variable regex #6019

2 of 20 tasks complete
@mbadams5

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2017

For posterity, the \retaincase ends up NOT affecting the ESO file and outputs. Everything is still upper-cased, so there is no need to update the output rules doc.

@jmarrec

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2017

Major +1 @mbadams5! Thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.