Skip to content

Conversation

@jasondegraw
Copy link
Member

@jasondegraw jasondegraw commented Apr 22, 2022

Pull request overview

The current native CSV output is much faster than but not as flexible as the outputs that can be produced by the ReadVars post-processing workflow. Unit options are limited and there is no analog of the RVI/MVI file. Develop a replacement for RVI/MVI input that allows the user to select units, ordering of outputs, and other reporting options that will allow ReadVars users to transition to the built-in CSV workflow.

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

@jasondegraw jasondegraw added the NewFeature Includes code to add a new feature to EnergyPlus label Apr 22, 2022
## References ##

---

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks perfectly reasonable. I was thinking if the csv is not created/requested and these new objects are present in the input then the "unused objects" message will show up in the err file, and if the csv is created/requested those messages will not show up.

OutputControl:Files,
    No,                     !- Output CSV

@JasonGlazer
Copy link
Contributor

Overall this is a great change.

a) Perhaps the input object names "OutputControl:RVI" and "OutputControl:MVI" should be more descriptive such as "OutputControl:CSV:SelectedOutputVariableOrder" and "OutputControl:CSV:SelectedMeterOrder" or something.

b) I believe that even if the RVI or MVI had a column that was not an existing variable that it would leave a blank column for that variable (someone else maybe can confirm this) so that the spreadsheet that is created had a fixed set of columns. Is this still going to occur?

c) Are there any other features of ReadVARSESO that are not supported? If so, please identify them.

d) ConvertESOMTR had this unique conversion to IP approach that worked with the names of the output variables. If it was output in Joules and the name contained the string "elec" it was converted to kWh, if it contained the string "gas" it was converted to therms, if it contained "cool" it was converted to ton-hr. Similarly, the output was Watts and the name contained "elec" then it was converted to kW. This is better than just converting all JtokWH as an option. I suggest that you add "JtoAuto" option in "OutputControl:CSV:Style" to replicate this.

e) Related to the last item, ConvertESOMTR used a "convert.txt" file to specify the rules of conversions which would allow users to modify the rules. I presume we are losing this capability. I don't think this is a big deal since I think it rarely (or maybe never) happened, but I just want to double-check that the "convert.txt" file is not going to be used anymore.

f) There is a little-used program called CSVproc that creates some simple statistics for each column in the CSV file. It would be great if this could just be done as part of this work (it is just the sum, max, min, average plus some counts) and this could be an added option to "OutputControl:CSV:Style" like "Generate simple statistics" maybe these are added to the bottom of the CSV file? It would be nice to eliminate this little program as well.

g) Are we going to eliminate ReadVARSESO and ConvertESOMTR from the installer? If so this may necessitate other changes to EP-Launch 2 source code and EP-Launch 3 workflows. Post an issue pointing to me about this if that is what is decided.

h) One other suggestion, Excel locks the CSV file so that it cannot be written to but it is very common for users to try to run EnergyPlus with the CSV opened by accident. EP-Launch detects this and shows a warning but maybe EnergyPlus should also do something initially when processing the command line options (maybe it does already) and just put a message up and not bother with the simulation.

@mjwitte
Copy link
Contributor

mjwitte commented Apr 25, 2022

If you want to be feature-complete with ReadVarsESO, there is also an option to filter output variables by reporting frequency. e.g. ReadVarsESO.exe my.rvi timestep will pass through only selected variables which have been requested at the timestep reporting frequency. ExerciseOutput1-CustomCSV.bat is an example that calls ReadVarsESO multiple times to create a separate csv output for each reporting frequency. One way to support this might be additional fields in the proposed OutputControl:CSV:Style object:

 A2, \field Output Variable Reporting Frequency
       \type choice
       \key AllCombined - default, all reporting frequencies combined in the same csv output file
       \key AllSeparated - each reporting frequency sent to a separate csv output file, e.g. eplusout_hourly.csv
       \key Hourly - only variables with hourly reporting frequency will be included in the csv output file
       \key Daily
       \key Monthly
       \key RunPeriod
       \key Environment
       \key Annual
       \default AllCombined 
 A3; \field Meter Variable Reporting Frequency

@mjwitte
Copy link
Contributor

mjwitte commented Apr 25, 2022

Regarding item b) above, ReadVarsESO does not leave blank columns if a variable is name in the rvi is not present in the idf Output:Variable or Output:Meter objects.

Also, in case you don't realize this, ReadVarsESO matches initial substrings in the variable name. For example "Zone Air" in the rvi will match all occurrences of Zone Air Temperature, Zone Air System Sensible Cooling Rate, etc. Not sure if it's important to support this, but noting it's a feature.

@JasonGlazer
Copy link
Contributor

Regarding item b) above, ReadVarsESO does not leave blank columns if a variable is name in the rvi is not present in the idf Output:Variable or Output:Meter objects.

Thanks for clarifying that.

@jasondegraw
Copy link
Member Author

@JasonGlazer thanks for the feedback! Here are responses to most of your comments, we'll drop b) because @mjwitte has responded on that.

a) Perhaps the input object names "OutputControl:RVI" and "OutputControl:MVI" should be more descriptive such as "OutputControl:CSV:SelectedOutputVariableOrder" and "OutputControl:CSV:SelectedMeterOrder" or something.

We're fine with the names you're proposing, it makes sense to drop the "RVI" and "MVI" terminology.

c) Are there any other features of ReadVARSESO that are not supported? If so, please identify them.

As far as we know, the things missing are mentioned in Mike's comment:

  • String matching
  • Frequency filtering

d) ConvertESOMTR had this unique conversion to IP approach that worked with the names of the output variables. If it was output in Joules and the name contained the string "elec" it was converted to kWh, if it contained the string "gas" it was converted to therms, if it contained "cool" it was converted to ton-hr. Similarly, the output was Watts and the name contained "elec" then it was converted to kW. This is better than just converting all JtokWH as an option. I suggest that you add "JtoAuto" option in "OutputControl:CSV:Style" to replicate this.

We are not terribly opposed to including this, but we are concerned that this will push our budget/time limits. If it isn’t a “must have”, then perhaps it could be a stretch goal? If this is a “must have”, we could combine it with e) as a FY23 new feature to guarantee it gets done.

e) Related to the last item, ConvertESOMTR used a "convert.txt" file to specify the rules of conversions which would allow users to modify the rules. I presume we are losing this capability. I don't think this is a big deal since I think it rarely (or maybe never) happened, but I just want to double-check that the "convert.txt" file is not going to be used anymore.

This is probably a bit out of our original scope, but we would not envision using a separate file if we did implement it. A potential solution would be to combine it with d) above for FY23.

f) There is a little-used program called CSVproc that creates some simple statistics for each column in the CSV file. It would be great if this could just be done as part of this work (it is just the sum, max, min, average plus some counts) and this could be an added option to "OutputControl:CSV:Style" like "Generate simple statistics" maybe these are added to the bottom of the CSV file? It would be nice to eliminate this little program as well.

We are reluctant to include additional “special” rows (beyond headers) based upon (very emphatic) user feedback. This seems like something that would still be best done as a postprocessing step (which would mean keeping CSVProc, but the program could be translated to Python).

g) Are we going to eliminate ReadVARSESO and ConvertESOMTR from the installer? If so this may necessitate other changes to EP-Launch 2 source code and EP-Launch 3 workflows. Post an issue pointing to me about this if that is what is decided.

We do believe that these programs should be deprecated at some point, but not in this PR. We would like to consider redirecting the --readvars command line option to use this new feature as part of this PR. We're not sure how that would impact EP-Launch, so there definitely needs to be discussion.

h) One other suggestion, Excel locks the CSV file so that it cannot be written to but it is very common for users to try to run EnergyPlus with the CSV opened by accident. EP-Launch detects this and shows a warning but maybe EnergyPlus should also do something initially when processing the command line options (maybe it does already) and just put a message up and not bother with the simulation.

E+ will probably detect that there’s something going on when it can’t open the file for writing, but we’ll have to look into what it does then. We think that handling it as a “failure to open output file” error is probably the way to go, other options may get platform specific really quickly.

@jasondegraw
Copy link
Member Author

@mjwitte thanks for the comments and feedback!

If you want to be feature-complete with ReadVarsESO, there is also an option to filter output variables by reporting frequency. e.g. ReadVarsESO.exe my.rvi timestep will pass through only selected variables which have been requested at the timestep reporting frequency. ExerciseOutput1-CustomCSV.bat is an example that calls ReadVarsESO multiple times to create a separate csv output for each reporting frequency. One way to support this might be additional fields in the proposed OutputControl:CSV:Style object:

We were aware of this capability, and we had looked into a little, but hadn't decided whether we should implement it or not. We'll take a harder look, and if it is feasible we'll do something. I don't know if that would necessarily mean multiple files or not, we'll just have to see how it plays out.

Also, in case you don't realize this, ReadVarsESO matches initial substrings in the variable name. For example "Zone Air" in the rvi will match all occurrences of Zone Air Temperature, Zone Air System Sensible Cooling Rate, etc. Not sure if it's important to support this, but noting it's a feature.

We had missed this in the review of ReadVars, thanks for catching this. This can probably be handled using the code that does matching for the output variables themselves, and if that can be done then this should be (relatively) easy to do. We’ll look into it and if it is feasible, we’ll do it.

@nrel-bot
Copy link

nrel-bot commented Jun 2, 2022

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

2 similar comments
@nrel-bot-3
Copy link

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

@nrel-bot
Copy link

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

@Myoldmopar
Copy link
Member

@jasondegraw do you anticipate this progressing for I/O freeze? I'm just scanning PRs looking at milestones now while prepping for the review push over the next couple weeks. Thanks! (If I repeat this comment on any other PRs, my apologies for cross posting.)

@Myoldmopar Myoldmopar added this to the EnergyPlus 23.1 IOFreeze milestone Aug 1, 2022
@nrel-bot-3
Copy link

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

1 similar comment
@nrel-bot-2
Copy link

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

@nrel-bot
Copy link

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

3 similar comments
@nrel-bot-2
Copy link

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

@nrel-bot
Copy link

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

@nrel-bot
Copy link

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

@Myoldmopar
Copy link
Member

@jasondegraw I saw you made a commit here. Are you planning on pushing on this in the very near term to get this into the 23.1 IO freeze next week?

@jasondegraw
Copy link
Member Author

@Myoldmopar Yes, working on it right now.

@nrel-bot-2
Copy link

@jasondegraw it has been 14 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 9 days since this pull request was last updated.

1 similar comment
@nrel-bot-2
Copy link

@jasondegraw it has been 9 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 8 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 14 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 12 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 8 days since this pull request was last updated.

1 similar comment
@nrel-bot-2
Copy link

@jasondegraw it has been 8 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 10 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 12 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 20 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 14 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 24 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

14 similar comments
@nrel-bot-2c
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 9 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@jasondegraw it has been 7 days since this pull request was last updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Development

Successfully merging this pull request may close these issues.