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

Extra Output Variables in v8.7 #6106

Closed
jonwinkler opened this issue Apr 20, 2017 · 7 comments
Closed

Extra Output Variables in v8.7 #6106

jonwinkler opened this issue Apr 20, 2017 · 7 comments
Assignees
Labels
PriorityHigh This defect or feature has been declared high priority SeverityHigh This defect is high severity, generally indicating a hard crash without a workaround

Comments

@jonwinkler
Copy link

Despite including a Key Value in certain Output:Variable objects, all corresponding output variables are being written (as if an * was included for the key value). This does not occur in v8.6.

This only occurs for certain outputs. See attached IDF and CSV files. For example, the Zone Mean Air Temperature is being written for all zones when it should only be writing it for 'living_1'. This is not happening for all output variables but I've discovered two so far (see output in attached ZIP file).

Attached is ZIP file containing two IDFs and two CSV files. A diff on the IDFs show they're the same file excluding the IDD changes from 8.6 to 8.7. The output from 8.7 has extra unwanted columns. This an issue when running 10,000's of simulations when output file size is a concern.

@nmerket

files.zip

@mjwitte mjwitte added PriorityLow This defect or feature has been declared low priority SeverityHigh This defect is high severity, generally indicating a hard crash without a workaround labels Apr 20, 2017
@mjwitte
Copy link
Contributor

mjwitte commented Apr 20, 2017

@mbadams5

@mbadams5
Copy link
Contributor

At first glance, it seems like it is adding a * key value for certain output variables and the culprit is here. These are added due to the following in your input file which then causes all those keys to be added internally.

Output:Table:SummaryReports,
   AllSummaryAndMonthly;

What I haven't figured out yet, is why E+ 8.6 did not have this issue. The new code in E+ 8.7 is working properly, it just "thinks" there are extra Output:Variables added to the IDF. I'll keep looking into this more and try to find a solution.

@mjwitte
Copy link
Contributor

mjwitte commented Apr 20, 2017

@JasonGlazer ?

@mjwitte
Copy link
Contributor

mjwitte commented Apr 20, 2017

@mbadams5 I can't help thinking the problem is that the re2 matching is being applied at the wrong place and it's expanding in the wrong direction when it sees the "*" in OutputVariablesForSimulation rather than expanding based on ReqRepVars.

@mbadams5
Copy link
Contributor

@mjwitte I don't fully understand what you mean when you say it's expanding in the wrong direction when it sees the "*" in OutputVariablesForSimulation rather than expanding based on ReqRepVars.
The regex matching is being done in the same exact place as the previous linked list search and as far as I am aware is doing the exact same matching, but obviously something is different.

@mjwitte
Copy link
Contributor

mjwitte commented Apr 21, 2017

@mbadams5 Neither do I - just fishing for an explanation. I don't fully understand all the output setup functions, but what I see at this point are essentially three passes (figuring this out as I go):

  1. To activate accumulation of all the required values (and avoid accumulating variables that are never used for anything). This pass goes through idf output variable requests in InputProcessor::PreScanReportingVariables and then it calls InputProcessor::AddVariablesForMonthlyReport which you identified earlier. Both of these call InputProcessor::AddRecordToOutputVariableStructure which ultimately sets up DataOutputs::OutputReportingVariables. At this point, the KeyValues in OutputReportingVariables include *s or strings from idf output objects.

  2. The next pass sets up OutputProcessor::ReqRepVars with a list of requested outputs from the idf objects. This happens in OutputProcessor::GetReportVariableInput. Important to note here that if the idf request has "*" for the key, ReqRepVars( Loop ).Key is set to a blank string, so later this is checked to see if it's empty or not.

  3. The final pass happens whenever OutputProcessor::SetUpOutputVariable. SetUpOutputVariable is called once for every possible unique key for a given variable name. So when it calls CheckReportVariable it is passing a "hard" string in KeyedValue that expects to find either an exact match or a "*" in OutputReportingVariables. If either of these conditions are true, then that variable is added to ` with some extra work to allow to multiple reporting frequencies and avoid duplicates.

    a. First, it checks to see if the variable name is in the list, if not, we're done.
    b. Then it calls BuildKeyVarList and AddBlankKeys.
    c. BuildKeyVarList loops through ReqRepVars skipping any that have a blank key (those are handled in AddBlankKeys). Then it calss DataOutputs::FindItemInVariableList to check for a match. At this point KeyedValue is a full name passed in from the call to SetUpOutputVariable.

I think I see the problem. In BuildKeyVarList prior to the regex changes, this line is comparing KeyedValue with ReqRepVars.

if ( ! SameString( ReqRepVars( Loop ).Key, KeyedValue ) ) continue;

But the same place in the current code is calling FindItemInVariableList which is comparing against OutputReportingVariables instead of ReqRepVars.

if ( ! DataOutputs::FindItemInVariableList( KeyedValue, VariableName ) ) continue;

So, I think the fix is to write a similar function to FindItemInVariableList that searches ReqRepVars, or change FindItemInVariableList to have another argument that passes a pointer to the desired search array.

shorowit referenced this issue in NREL/OpenStudio-workflow-gem Apr 22, 2017
@mjwitte mjwitte added PriorityHigh This defect or feature has been declared high priority and removed PriorityLow This defect or feature has been declared low priority labels May 24, 2017
@JasonGlazer JasonGlazer self-assigned this Sep 8, 2017
@Myoldmopar
Copy link
Member

Closed via #6303

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PriorityHigh This defect or feature has been declared high priority SeverityHigh This defect is high severity, generally indicating a hard crash without a workaround
Projects
None yet
Development

No branches or pull requests

5 participants