Refactor array-based input code to use key-based input (part 2)#11526
Refactor array-based input code to use key-based input (part 2)#11526
Conversation
jasondegraw
added
DoNotPublish
|
|
|
|
amirroth
left a comment
amirroth
left a comment
There was a problem hiding this comment.
It's about time we did some of this. Thanks @jasondegraw.
| IOStatus); | ||
| if (!Util::SameString(name_to_find, state.dataIPShortCut->cAlphaArgs(1))) { | ||
| for (auto const &modeInstance : modeInstances.value().items()) { | ||
| auto const modeName = Util::makeUPPER(modeInstance.key()); |
There was a problem hiding this comment.
I'm not a fan of using auto for std::string, but in your defense, at least you are using auto and auto & correctly. 🥇
|
|
|
|
|
|
|
|
|
|
|
|
|
Something went wrong with the changes to CostEstimateManager.cc, git was acting funny and I didn't realize something was up until after a push. So this is the last attempt to fix this one before I just revert those changes out.
|
|
|
|
|
|
| if (dehumidObjects != inputProcessor->epJSON.end()) { | ||
| for (auto const &dehumidInstance : dehumidObjects.value().items()) { | ||
| auto const &dehumidFields = dehumidInstance.value(); | ||
| auto const dehumidName = Util::makeUPPER(dehumidInstance.key()); |
There was a problem hiding this comment.
I've said this before, not a fan of using auto for "scalar" types like int, Real64 or std::string.
jasondegraw
left a comment
jasondegraw
left a comment
There was a problem hiding this comment.
The modifications here all pretty much follow the same pattern, with some of the modules being a little more convoluted than others. These files are all pretty straightforward, there are more complex cases in the ~140 or so more to go.
|
|
||
| #include <EnergyPlus/Data/EnergyPlusData.hh> | ||
| #include <EnergyPlus/DataHeatBalance.hh> | ||
| #include <EnergyPlus/DataIPShortCuts.hh> |
There was a problem hiding this comment.
I think I'll only do comments on the first one since the pattern is all pretty much the same. If possible, remove the use of DataIPShortCuts.
| static constexpr std::string_view routineName = "GetHysteresisData"; | ||
|
|
||
| auto &s_ipsc = state.dataIPShortCut; | ||
| auto &s_ip = state.dataInputProcessing->inputProcessor; |
There was a problem hiding this comment.
The use of shortcuts is largely preserved, added to in some places, but the refactors should largely follow the original intent of the code at hand.
| return; | ||
| } | ||
|
|
||
| for (auto const &hysteresisInstance : hysteresisObjects.value().items()) { |
There was a problem hiding this comment.
There's generally going to be a loop like this, and this is where the order is getting changed. The array-based processing is moving through the values differently than the key-based code, and so will register their membership in things like meters, etc. in a different order.
|
|
||
| for (auto const &hysteresisInstance : hysteresisObjects.value().items()) { | ||
| auto const &hysteresisFields = hysteresisInstance.value(); | ||
| auto const materialName = Util::makeUPPER(hysteresisInstance.key()); |
There was a problem hiding this comment.
There are some issues that have cropped up with upper/lower casing, and it would be great if we could drop the case insensitivity at some point. I'll argue that it really most meaningful for hand-editing models.
| if (mat->hasPCM) { | ||
| ShowSevereCustom( | ||
| state, eoh, EnergyPlus::format("Material {} already has {} properties defined.", mat->Name, s_ipsc->cCurrentModuleObject)); | ||
| ShowSevereCustom(state, eoh, EnergyPlus::format("Material {} already has {} properties defined.", mat->Name, currentModuleObject)); |
There was a problem hiding this comment.
I have not checked whether all of these checks are really needed. This one looks like it might be, but there are a lot of things now that are caught by the validator and never make it into the input processing code.
| matPC->deltaTempFreezingHigh = s_ipsc->rNumericArgs(11); | ||
| matPC->peakTempFreezing = s_ipsc->rNumericArgs(12); | ||
| matPC->deltaTempFreezingLow = s_ipsc->rNumericArgs(13); | ||
| matPC->totalLatentHeat = |
There was a problem hiding this comment.
There's generally a big block of lines where the array mining is replaced with the appropriate convenience getter functions.
Thanks @jasondegraw. Hang in there :) This one merges next. |
|
LGTM. Merging. |
4 participants
Pull request overview
This is a follow-up PR that replaces array-based input code with key-based input code. The first one went well, so full speed ahead. No thought went into the files that were chosen other than to try and limit the scope of changes. The refactor failed in a couple of cases in the sense that the modifications led to numerical diffs (BaseboardElectric) or something went south on a merge (CostEstimateManager), so I yanked those changes back out and will work on them later.
I believe that all of the remaining diffs are related to the order in which things are read in during the input processing. The summary is as follows:
These are the same across the different platforms. Here's a table of the models with the diffs:
Here's the MTD diff for 5ZoneEndUses:
and the EIO from MicroCogeneration:
All of the items for which a proper diff is included are of this form. The "table string diffs" are nearly the same, except that there isn't enough in the regression download to fully investigate those. Here's a subset of the plain old diff that I did on local runs with this branch versus develop:
Same data, different order.
Everything I have checked is the same as these examples. If there's an interest in others I can reproduce them here. One question that came up: why aren't we including more of the table diffs in the download? That would have made checking things out a bit easier.
Pull Request Author
Reviewer