-
Notifications
You must be signed in to change notification settings - Fork 401
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
Fix 9-3-0 to 9-4-0 transition for objects that accept report variable or meter name #8366
Conversation
Looks fine to me. The test file transitioned successfully and runs with current develop. I'm not planning on running this new transition utility with the entire gamut of 9.3->9.4 transitions tests like we did right before release, but I suppose I could if it's absolutely necessary. @mjwitte feel free to add comments and merge when ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dareumnam Looking more closely at this, the real problem is here
CurVar=4 ! In case Source Meter would change
DO Var=4,CurArgs,2
The 4s should be 5s here. This DO loop will make the correct replacements from the list in Report Variables 9-3-0 to 9-4-0.csv
IF (changeMeterNameFlag) THEN | ||
CALL ReplaceFuelNameWithEndUseSubcategory(OutArgs(3), NoDiff) | ||
END IF | ||
END IF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The special section for 9.4 is good up to here - this catches any changes needed for field A3 Source Meter Name (and avoids breaking the name if it came from a user-defined custom meter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review. I'll update the code and push it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to chip in. But shouldn't the 4s be 3s here? The 3rd field is Source Meter Name
and its value should also be updated according to Report Variables 9-3-0 to 9-4-0.csv?
CurVar=4 ! In case Source Meter would change
DO Var=4,CurArgs,2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hongyuanjia No. Field 3 is handled in it's own block, lines 1224:1235.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjwitte, thanks for the clarification. So only the source meter name will be changed if necessary. Previously I was thinking this code block should replace fuel names for all possible meters, including those listed Output Variable or Meter Name X
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hongyuanjia You may be right. I suppose a custom meter could reference other custom meters or a user-defined end-use meter. @dareumnam will build test files for more use-cases.
CALL ReplaceFuelNameWithEndUseSubcategory(OutArgs(3), NoDiff) | ||
END IF | ||
END IF | ||
DO WHILE (meterCustomDecrName <= 47) ! Output Variable or Meter Name 1 to 22 (A5, A7, A9, ... A47) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This DO loop is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
@@ -1472,3 +1493,24 @@ SUBROUTINE ReplaceFuelNameWithEndUseSubcategory(InOutArg, NoDiffArg) | |||
NoDiffArg=.false. | |||
END IF | |||
END SUBROUTINE | |||
|
|||
SUBROUTINE ReplaceElectricityMeterName(InOutArg, NoDiffArg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This subroutine is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Thanks @dareumnam for the quick fix, and thanks @mjwitte for the code review. I've tested it again and it looks good. Anything else to do here before this goes in? |
@mitchute I think that's all for this object. Looks like we may need add the special 9.4 meter name piece to EnergyManagementSystem:Sensor, DemandManagerAssignmentList, and ElectricLoadCenter:Distribution, because these can all reference a meter name. |
This is ready for review.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with revised defect file. Looks good.
Pull request overview
Meter:Custom
,Meter:CustomDecrement
,EnergyManagementSystem:Sensor
,DemandManagerAssignmentList
,ElectricLoadCenter:Tranasformer
,ElectricLoadCenter:Distribution
,UtilityCost:Tariff
)ASHRAE901_OfficeMedium_STD2019_Denver.idf.txt
Meter:Custom
Meter:CustomDecrement
EnergyManagementSystem:Sensor
DemandManagerAssignmentList
ElectricLoadCenter:Tranasformer
ElectricLoadCenter:Distribution
UtilityCost:Tariff
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.