Correction of Enclosure Based Solar Output Variables #10563

RKStrand · 2024-06-17T20:15:58Z

Pull request overview

Fixes "Zone Exterior Windows ..." output variables confusingly take a Space name #10552
A series of 10 variables were misnamed as "Zone" variables when in fact they should have been labeled as "Enclosure" variables. This led to confusion for users who thought that specifying a zone name for the output variable declarations would produce output. In fact, when the enclosure and zone name are not the same, the output would not be produced. This corrects the problem. Solution includes code modification for the output variable names correction, updating numerous output files in the test suite to use the correct name (idf and rvi), various transition files/code, and documentation updates. No unit test (by permission) since this did not change executable code. Differences in the output are all due to the variable name change.

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.

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
[NO] 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.

Renamed variables required to correct the issue with Defect #10552 Includes modified source code including table reports and modified input files in the testfiles folder.

Modification of the documentation for the variable name changes. Also found two variables that were not previously shown in the list. Plus added some text about where to find the enclosure name if the user is uncertain what it is.

Modifications to the transition program and the transition report variables file for the name changes associated with this correction.

It was not noticed that the energy management sensor was already showing up in the CASE list, leading to a duplicate CASE which the compiler did not appreciate.

Missed a couple of IDFs that had variables that were changed.

mjwitte · 2024-06-19T18:50:45Z

src/Transition/CreateNewIDFUsingRulesV24_2_0.f90

@@ -500,6 +500,46 @@ SUBROUTINE CreateNewIDFUsingRules(EndOfFile,DiffOnly,InLfn,AskForInput,InputFile
                  OutArgs(1)='*'
                  nodiff=.false.
                ENDIF
+                IF (OutArgs(3) == 'ZONE WINDOWS TOTAL TRANSMITTED SOLAR RADIATION RATE') THEN


These lines shouldn't be necessary. Transition should read Report Variables 24-1-0 to 24-2-0.csv and convert everything on the list (for both output variables and EMS sensors).

Got it--my bad. I have backed this out in the latest commit that I just made.

mjwitte · 2024-06-19T18:53:13Z

src/Transition/SupportFiles/Report Variables 24-1-0 to 24-2-0.csv

-0,0,These numbers should be the number of report variables in the following list (including deletes). Two columns/numbers.
-
+24.1.0,24.2.0,Transition notes - some of these are EMS variable names
+0,0,These numbers should be the number of report variables in the following list (including deletes). Two columns/numbers.


Should be 10,10,These numbers......

Also fixed in the latest commit that I just made.

mjwitte · 2024-06-19T18:54:48Z

testfiles/_SmallOffice_Dulles.idf

@@ -3570,13 +3570,13 @@

  Output:Variable,*,Surface Window Transmitted Solar Radiation Rate,annual;

-  Output:Variable,*,Zone Exterior Windows Total Transmitted Beam Solar Radiation Rate,monthly;
+  Output:Variable,*, Enclosure Exterior Windows Total Transmitted Beam Solar Radiation Rate,monthly;


I'm surprised at how many test files had some of these output variables in them.

mjwitte · 2024-06-19T19:23:27Z

testfiles/_SmallOffice_Dulles.idf

@@ -5057,9 +5057,9 @@
    SumOrAverage,            !- Aggregation Type for Variable or Meter 2
    Zone Windows Total Transmitted Solar Radiation Rate,  !- Variable or Meter 3 Name


Missed some conversions here (AUD diffs were the hint something was up). There are also AUD diffs in 5ZoneEconomicsTariffAndLifeCycleCosts, 5ZoneTDV, and several other files.

@Myoldmopar When I run this file locally, I see err diffs. There's a new warning with this branch, but no err diffs on ci:

** Warning ** Processing Monthly Tabular Reports: Variable names not valid for this simulation ** ~~~ ** ...use Output:Diagnostics,DisplayExtraWarnings; to show more details on individual variables.

And, duh, just realized, that's because locally it ran a RunPeriod simulation, but CI didn't, so these monthly reports didn't process.

Huh, weird. Ok, I see that somehow I missed some of these IDF files. Not sure how, unless these were modified during recent commits. But it is strange that it didn't show up in the CI. Anyway, I'm working my way through this again. Should have another commit to fix these issues today.

Ok, just made another commit to address this. Should be all of them now. Thanks!

Backed out changes that were not need in the transition program due to the variables being listed in the .csv file. Also made a correction to the report variables .csv file.

During review, it was noticed that there were new error messages in various files. Apparently, not all of the IDFs with the old variable names were converted to the new names. This should now be corrected.

Not sure how these keep getting missed. Probably user error...

mjwitte

Tested transition - works well now. Found a couple of typos and cleaned up some unrelated warnings in another idf. Changes pushed. Once CI comes back clean, will merge.

"just change the names of some output variables" Such a simple fix - not.
Thanks @RKStrand

mjwitte · 2024-06-20T20:41:07Z

testfiles/1ZoneUncontrolled_win_2.idf

-! Multiplier 2 causes a new window definition.
-
-  Output:Variable,ZN001:WALL001:WIN001:2,Surface Outside Face Incident Solar Radiation Rate per Area,timestep;


Just for the record. Window multipliers do not create new windows, they just adjust the areas. Delete these output variable objects that trip warnings .
** Warning ** The following Report Variables were requested but not generated -- check.rdd file

RKStrand added 7 commits June 12, 2024 17:50

Variable Renames for Defect 10552

6dd7c81

Renamed variables required to correct the issue with Defect #10552 Includes modified source code including table reports and modified input files in the testfiles folder.

Doc mods for Defect 10552

e7a36b3

Modification of the documentation for the variable name changes. Also found two variables that were not previously shown in the list. Plus added some text about where to find the enclosure name if the user is uncertain what it is.

Transition Updates for Defect 10552

8c3a426

Modifications to the transition program and the transition report variables file for the name changes associated with this correction.

Merge branch 'develop' into 10552EnclosureOutputVariableNameCorrection

e384541

Fix of transition program error

687132c

It was not noticed that the energy management sensor was already showing up in the CASE list, leading to a duplicate CASE which the compiler did not appreciate.

Merge branch 'develop' into 10552EnclosureOutputVariableNameCorrection

45fa8ab

Final Edits for 10552

109fedc

Missed a couple of IDFs that had variables that were changed.

RKStrand added Defect Includes code to repair a defect in EnergyPlus OutputChange Code changes output in such a way that it cannot be merged after IO freeze labels Jun 17, 2024

RKStrand added this to the EnergyPlus 24.2 IOFreeze milestone Jun 17, 2024

RKStrand requested review from Myoldmopar and mjwitte June 17, 2024 20:15

RKStrand self-assigned this Jun 17, 2024

mjwitte reviewed Jun 19, 2024

View reviewed changes

RKStrand and others added 5 commits June 20, 2024 06:59

Corrections per Review for PR10563

0367c4a

Backed out changes that were not need in the transition program due to the variables being listed in the .csv file. Also made a correction to the report variables .csv file.

Fix Name Change in More IDFs PR10563

d5f7af9

During review, it was noticed that there were new error messages in various files. Apparently, not all of the IDFs with the old variable names were converted to the new names. This should now be corrected.

More missed variables PR10563

305c8bb

Not sure how these keep getting missed. Probably user error...

Typos

828f346

Unrelated remove bad output variables

2df72ef

mjwitte approved these changes Jun 20, 2024

View reviewed changes

mjwitte reviewed Jun 20, 2024

View reviewed changes

mjwitte merged commit 1d1665a into develop Jun 20, 2024
15 checks passed

mjwitte deleted the 10552EnclosureOutputVariableNameCorrection branch June 20, 2024 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correction of Enclosure Based Solar Output Variables #10563

Correction of Enclosure Based Solar Output Variables #10563

RKStrand commented Jun 17, 2024 •

edited by mjwitte

Loading

mjwitte Jun 19, 2024

RKStrand Jun 20, 2024

mjwitte Jun 19, 2024

RKStrand Jun 20, 2024

mjwitte Jun 19, 2024

mjwitte Jun 19, 2024

RKStrand Jun 20, 2024

RKStrand Jun 20, 2024

mjwitte left a comment

mjwitte Jun 20, 2024

		@@ -5057,9 +5057,9 @@
		SumOrAverage, !- Aggregation Type for Variable or Meter 2
		Zone Windows Total Transmitted Solar Radiation Rate, !- Variable or Meter 3 Name

		! Multiplier 2 causes a new window definition.

		Output:Variable,ZN001:WALL001:WIN001:2,Surface Outside Face Incident Solar Radiation Rate per Area,timestep;

Correction of Enclosure Based Solar Output Variables #10563

Correction of Enclosure Based Solar Output Variables #10563

Conversation

RKStrand commented Jun 17, 2024 • edited by mjwitte Loading

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjwitte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKStrand commented Jun 17, 2024 •

edited by mjwitte

Loading