Eliminate Input Order Dependency in AirflowNetwork Linkage Objects#11148
Eliminate Input Order Dependency in AirflowNetwork Linkage Objects#11148
Conversation
The input for AirflowNetwork:Distribution:Linkage objects required that the first instance had a node that was an EnergyPlus node rather than just a node within the AFN. Thus, the input was order dependent--something that E+ says it's not. This fix reorders the reading of the input object so that the first one it reads is now one that has an EnergyPlus node. The user input file that previously received a severe/fatal now runs successfully and has output that matches a version of the input file where the objects didn't violate the order issue.
A unit test was created to test the new integer function that determines the shift in input. In addition, since the order of the AFN linkage statements is now changed internally, several of the unit tests for AFN had to have hard-wired indices adjusted to reflect the new order.
Reordering caused some crashes in ci and this revealed that the new subroutine was missing a condition. This was fixed, the new unit test was modified/strengthened, and the existing unit tests that were changed in the last commit were backed out.
| } | ||
| } | ||
| return shiftResult; | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jasondegraw
left a comment
There was a problem hiding this comment.
The issue does appear to be legitimate, and this PR does appear to fix the issue, but it's adding additional array-based input processing. I hate to be "that guy", but this really needs to be done with the JSON-based input processing, otherwise it'll have to be redone later.
|
@jasondegraw I have a question regarding your comment about this needing to be done in JSON as well. Now, I'll be the first to admit that I'm not 100% up to speed on JSON yet, so if this question is dumb, at least you'll know why. Since this fix does not alter in any way reading the data from JSON or the IDF but simply looks at what was contained in the input file, how will the input file format matter? Isn't the get object routine simply going to one or the other but the data inside E+ is the same? Otherwise, wouldn't every object have two different paths? So, if this fix is simply looking at what has already been read in and then simply choosing to process the already read data into local arrays in a slightly different order, what does it matter whether the data came from a JSON file or an IDF? I understand the concept of not wanting to have to come back and redo this later, but given what I am proposing to do here, I don't comprehend why this would need to be redone later. Of course, I might not understand something about what JSON is doing. Thanks for your feedback. |
|
It's a valid request from @jasondegraw and a fine response from @RKStrand . I'll try to clarify a bit. Right now, EnergyPlus accepts input files in traditional IDF, or JSON (or some binary formats as well). Once inside EnergyPlus, the data is actually processed into a JSON-like object for any processing. Right now input processing routines can either:
this->soil.k = j["soil_thermal_conductivity"].get<Real64>();
this->soil.rho = j["soil_density"].get<Real64>();
this->soil.cp = j["soil_specific_heat"].get<Real64>();
thisPLHP.loadSideDesignVolFlowRate = state.dataInputProcessing->inputProcessor->getRealFieldValue(fields, schemaProps, "load_side_reference_flow_rate");
if (thisPLHP.loadSideDesignVolFlowRate == DataSizing::AutoSize) {
thisPLHP.loadSideDesignVolFlowRateWasAutoSized = true;
}
thisBoiler.VolFlowRate = s_ipsc->rNumericArgs(3);So why are you being asked to change this code? Well, we would love to eliminate the GetObjectItem layer, and access everything based off of keys. That requires going through every get input routine and converting it over, pretty much by hand. One caveat: when you access things directly off of the JSON data structure, you need to explicitly tell EnergyPlus that you have "used" this object instance. Otherwise there would be a warning at the end of the simulation that says "Object XYZ named ABC was not used". It is as simple as: state.dataInputProcessing->inputProcessor->markObjectAsUsed(cCurrentModuleObject, thisObjectName);So what to do here? Well, if you were just literally changing a line or an index or something, I would say maybe don't worry about converting it over to a key-value JSON-style input processing function. Since it's a new function entirely, and it's pretty small, I think it's a great lightweight opportunity for you to make the conversion, so that you know how to do it on future work. And it's one less function we have to convert later..... |
The previous solution worked for the current version, but with the move to JSON for input, a reviewer expressed concerns that this would have to be fixed when this input was transitioned to JSON. Since JSON reorders the input, a more comprehensive solution was needed. This commit converts the AirflowNetwork:Distribution:Linkage input to using JSON directly and the solution should work for any input that includes these objects in any order. So, conversion to JSON for this object is now complete as part of the solution.
|
@jasondegraw @Myoldmopar @rraustad Ok! I believe that I have successfully completed this fix. The input object that was causing the problem (AirflowNetwork:Distribution:Linkage) has been converted to using JSON. The solution is not pretty but it really has to be a bit methodical to make sure the AirLoopNum get propagated throughout the network despite where the "start" of the loop is entered/read into the order. This is a completely different solution than previously done with a new unit test as well. I think this addresses all of the concerns expressed. Please feel free to look through this when you have a chance and let me know if you see any issues. Thanks! |
|
|
@Myoldmopar Thanks so much for that explanation! I think I understand much better now and I can see where doing it for this fix would give me a chance to figure this out on a small scale so that in future work I'll be a little more up to speed. I have marked this as "waiting for developer" and will look to implement what @jasondegraw requested. (Oops...this was supposed to go out several days ago, but I forgot to click "Comment") Update: the work here is done and this was switched back to "Waiting on Reviewer". Hopefully, this adequately addresses the concerns expressed previously. |
jasondegraw
left a comment
There was a problem hiding this comment.
@RKStrand This will be better long term, thanks for being willing to go back over it.
|
@RKStrand I saw this was conflicted, so I merged in develop and resolved the conflict. I also noticed CI was failing so I made a debug build, and found a divide by zero in the The divide by zero is on line Solver.cc:12808, which I don't think you directly modified, but somehow the logic is allowing the SupplyBranchArea variable to be zero at this point. 
Let me know if you need anything else from me. |
|
(Also I just pushed the conflict resolved branch, so make sure to pull before attempting any pushes.) |
|
|
@RKStrand @Myoldmopar it has been 28 days since this pull request was last updated. |
Made some corrections to address issues that were present in the unit tests. Note to @rraustad: not 100% sure about the fix to line 12808/12803 but it seems wrong in the code? Also got rid of some temporary assignments that were added during debugging.
|
@mitchute I just resolved the conflict I think. I'm not sure why there was a conflict as I don't think I messed with anything here, but hopefully everything is fine now. |
|
|
…EnergyPlus into 10652OrderDependencyFixAFN
|
|
|
@RKStrand it looks like there's still a test failure here to resolve. Pointing it back to you for now. |
Interesting. Forgot to convert the name to uppercase which surprisingly became a problem only when dealing with duct loss stuff.
|
|
|
@RKStrand seeing some new err diffs here. ************* No node connection errors were found.
************* Beginning Simulation
************* Simulation Error Summary *************
+ ************* There are 18 unused objects in input.
+ ************* Use Output:Diagnostics,DisplayUnusedObjects; to see them.
*************
************* ===== Final Error Summary =====
************* The following error categories occurred. Consider correcting or noting. |
|
@mitchute Huh. Well, I'm not sure what is causing that. It's possible that making this one thing all caps caused other issues I guess. I'll have to track that down but I'm guessing it will take some time. That might mean this doesn't make the 26.2 release. |
|
Not sure. I just pulled in develop and we'll run it again here. I'll do some local testing in the meantime. |
|
|
@mitchute I took a further look at this and it appears that there is a "difference" in the way I handled the "mark object as used" call. Elsewhere in Solver.cpp, it sets the local data "name" to the upper case of "instance.key()" but then when it checks if the object is used, it uses the unmodified instance.key(). So...I'm going to pull your latest that merged in develop and then make this change to what I did. Maybe this will be simpler than I thought??? |
|
Hopefully this does the trick...
|
@mitchute Ok, I've pulled the latest with develop merged in and made that change. Hoping for a Halloween treat here and no more "tricks"! |
|
Just ran regressions locally, and it's all looking good so far. |
|
|
|
This is looking good. Those unused object diffs are now gone and the numerical diffs seem to be expected. We can merge this unless I hear otherwise from anyone. |
|
I was wondering why there were any diffs at all from these changes and then I reviewed and remembered you did this: 
|
Pull request overview
Description of the purpose of this PR
A user noted that an error message was being produced by EnergyPlus that indicated that some of the input for AirFlowNetwork inputs were not in the correct order. EnergyPlus has always functioned within the concept of the input can be in any order. So, this violated that principle. While there is an internal requirement within AFN that the first encountered Linkage object has a node that is connected to actual E+ HVAC equipment, this can be accommodated without enforcing that the input be in a particular order. This solution fixes the problem by searching throughout the data that has been read from input (now using JSON directly) until it finds an AirLoopNum that pertains to the air flow network. In testing, very tiny differences were noted in some sizing results. A unit test verifies the fix and several existing unit tests that used hard-wired indices for making comparisons were adjusted since the input is read in a slightly different order now.
Pull Request Author
Reviewer