Partial Fix for GetChildrenData Sort Order #11066
Conversation
|
| // No sub-component is connected to either the parent | ||
| // component's inlet- or outlet- nodes? Just copy the | ||
| // sub-components in the order in which they appear in | ||
| // the CompSets |
There was a problem hiding this comment.
Hmm, but this should never happen. The problem here is the OA mixer which has 2 inlets but only one entry in CompSets. MixedAir.cc::2012
BranchNodeConnections::TestCompSet(
state, CurrentModuleObject, state.dataMixedAir->OAMixer(OutAirNum).Name, AlphArray(3), AlphArray(2), "Air Nodes");
This is using "Outdoor Air Stream Node Name" as inlet and "Mixed Air Node Name" as outlet which makes sense when looking at the flow through OA system components, but not so much when it's in a parent unit like a PTHP or FanCoil. Adding another call here will likely break something else in the tangled web of CompSets and node connection checks. It might be possible to let the parent object(s) dictate the connection nodes.
For this case, adding the outlet node check works, because the last child component works here.
@amirroth I can tinker with this if you like. But maybe just take this fix and leave well-enough alone for now.
There was a problem hiding this comment.
For the record, I don't know if this fall-thru case where neither input-node or output-node match a child node happens in the wild or not (I guess we can print a warning and see if it shows up in .err). But this was effectively the fall-thru behavior when output-node was not checked so I kept it.
I can't speak to the tangled web of CompSets and will follow whatever recommendations you have.
There was a problem hiding this comment.
For the record, I don't know if this fall-thru case where neither input-node or output-node match a child node happens in the wild or not (I guess we can print a warning and see if it shows up in .err). But this was effectively the fall-thru behavior when output-node was not checked so I kept it.
Let's add a warning.
There was a problem hiding this comment.
Do we still need to add a warning here before this merges?
There was a problem hiding this comment.
Sorry, I did that locally but didn't pull/merge. Can you add that yourself please? Thanks.
| for (int Loop = 1; Loop <= NumChildren; ++Loop) { | ||
| if (ChildMatched(Loop)) continue; | ||
| ++CountNum; | ||
| ChildrenCType(CountNum) = ChildCType(Loop); |
There was a problem hiding this comment.
Spot checked random set of files that were showing diffs. VSHeatPumpWaterHeater with debug build fails here with a vector error, CountNum=5 but NumChildren=4.
There was a problem hiding this comment.
Hmmm. Why is this not showing up in the CI report? Or is it and I just don't know where to look?
There was a problem hiding this comment.
So this particular case crashes because there is a circularity in the node definitions:
- ParentInNodeName = "HPWHWATERINLETNODE"
- Component 2 "HPWHDXCOILVS", InNode = "HPWHWATERINLETNODE", OutNode = "HPWATEROUTLETNODE"
- Component 4 "HPWHTANK", InNode = "HPWATEROUTLETNODE", OutNode = "HPWHWATERINLETNODE"
The old code dealt with this issue by basically deleting sub-components from the temporary list if they have already been processed such that they could not be found again, and I could do something similar here. I assume this is a valid node configuration ...
There was a problem hiding this comment.
Yes, valid, intentional, as designed. The coil water nodes connect directly to the WH tank inlet/outlet nodes.
There was a problem hiding this comment.
Hmmm. Why is this not showing up in the CI report? Or is it and I just don't know where to look?
It didn't get caught in release builds on gha, and it looks like the linux CI tests are not running today. @Myoldmopar ?
But I'm not 100% certain it would have failed there either.
|
|
That file is fixed (and no longer has diffs along with a few others). I'll try to run a set of all files locally with a full debug build just to be sure this is all good. |
|
Running debug build locally on all test files, lots of files failing. 5ZoneAirCooledDemandLimiting, Oh, but that's failing in EconomicTariff with subscript error (also in Fault_FoulingEvapCooler_StripMallZoneEvapCooler, so maybe worth looking into these separately) . And AirflowWindowsAndBetweenGlassBlinds fails in optical calcs with an unitialized variable. Ok, this will take some time to sort out and fix the failures. Can't tell easily if any are related to this PR or even if they are all tripping on the same thing. |
|
They have nothing to do with this PR. The tariff problem snuck in with #11040 (sorry). Julien has a branch with a fix for it. I think it is called Followup_11040. The other one also sounds familiar, albeit older. How are these not picked up by CI? |
|
|
Alright, pulled in develop, resolved the conflicts, and applied formatting. I think this should produce some really meaningful CI results now. |
|
|
|
|
@mjwitte @amirroth @Myoldmopar it has been 33 days since this pull request was last updated. |
mjwitte
left a comment
There was a problem hiding this comment.
@Myoldmopar @amirroth Status update:
- Let's skip the warning for the fall-thru case. It would just confuse users and it's safer to leave the fall-thru code in place to cover use cases I haven't thought of.
- I re-ran the random set of test files that I was using before and everything is fine. With a debug build, there's an unrelated failure but that's for another PR on another day.
@Myoldmopar Ready for your final review.
|
Thanks @mjwitte, doing a quick final check here now. |
|
All happy here, merging in now. |
Pull request overview
Description of the purpose of this PR
Topological sort of child sub-components in a parent component was not functioning when parent component inlet-node did not match the inlet-node of any of the child components. This PR adds a backup case that sorts child components properly when the parent-component outlet-node matches the outlet node of one of the child components.
This is a partial fix because if neither parent inlet-node or outlet-node is matched the child components will be returned and displayed in the order in which they were registered into the CompSets list.
Pull Request Author
Reviewer