-
Notifications
You must be signed in to change notification settings - Fork 193
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
Coil:Heating:*: on and off cycle name changes #4970
Conversation
src/osversion/VersionTranslator.cpp
Outdated
// Change Parasitic Electric Load to On Cycle Parasitic Electric Load | ||
// Change Parasitic Gas Load to Off Cycle Parasitic Gas Load | ||
|
||
auto iddObject = idd_3_7_0.getObject(iddname); | ||
IdfObject newObject(iddObject.get()); | ||
|
||
for (size_t i = 0; i < object.numFields(); ++i) { | ||
if ((value = object.getString(i))) { | ||
newObject.setString(i, value.get()); | ||
} | ||
} | ||
|
||
m_refactored.push_back(RefactoredObjectData(object, newObject)); | ||
ss << newObject; |
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.
I don't think you need VT at all actually. The VT Test, good, we want to make sure.
But this is nothing but a name change, the fields stay the same. Try removing VT and check the test.
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.
I thought about that... but if we weren't changing them in the cpp get/set methods, so then how would the field names be changing because of default vt?
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.
I tried commenting out the new vt. And all tests seems to pass. So I'm pretty confused - default vt is or isn't changing the names of the fields?
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.
Alright, so, if you do nothing, the object is serialized as is.
Then the stringstream is reloaded with the 3.7.0 IDD. The IddObject is just gotten by its name, and the field values go to the idf object (no issue there),. The IddObject is the the bit that carries the field names (and field properties). And that's gotten from the new idd, so the field names are "updated".
Let me know if this isn't clear (I fear it isn't, but I'm aiming for quickness here).
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.
So, before, model tests were passing (even before changing in model cpp from, e.g., OS_Coil_Heating_GasFields::ParasiticElectricLoad
to OS_Coil_Heating_GasFields::OnCycleParasiticElectricLoad
) because was using old idd. Now it's using new idd, and vt serializes to it by default.
CI Results for 0b43f96:
|
Ok, don't worry about the failing SQLtests, I fixed those on #4972 |
Pull request overview
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.