AirflowNetwork Refactor #9429
AirflowNetwork Refactor #9429
Conversation
jasondegraw
added
the
Refactoring
| struct DetailedOpeningSolver | ||
| { | ||
| // Large opening variables | ||
| EPVector<Real64> DpProf; // Differential pressure profile for Large Openings [Pa] |
There was a problem hiding this comment.
More EPVector, less Array1D! Given that these are all similarly dimensioned, does it make sense to have a single EPVector of struct rather than three EPVector of Real64. Are these three quantities commonly used together?
There was a problem hiding this comment.
The detailed opening calculation didn't fare especially well in the transition to C++, so there's a lot to do on this. I wasn't sure if this solver needs to be it's own object, be part of the Solver object, or if the data and functions needed to be distributed to the DetailedOpening objects themselves. Because the detailed opening object gets used a lot, I have been reluctant to change too much, so I opted for the middle path here. I think the answer to the "are these used together" question is yes, but I'll know for sure once I dig into it more (which I am doing as part of the usability work).
| int const il, // Linkage number | ||
| int const Pprof, // Opening number | ||
| Real64 const G, // gravitation field strength [N/kg] | ||
| const Array1D<Real64> &DpF, // Stack pressures at start heights of Layers |
There was a problem hiding this comment.
EPVectors? EPVector of struct rather than six EPVector?
There was a problem hiding this comment.
Yes, I believe so, working on verifying.
|
|
||
| void lclimb(EnergyPlusData &state, | ||
| Real64 const G, // gravity field strength [N/kg] | ||
| Real64 &Rho, // Density link level (initialized with rho zone) [kg/m3] |
| Real64 density(Real64 P, // Barometric pressure | ||
| Real64 T, // Temperature in Celsius | ||
| Real64 W // Humidity ratio | ||
| ); |
There was a problem hiding this comment.
I don't think I've ever seen this style of closing paren on its own line a la closing brace.
| Real64 P // Barometric pressure | ||
| ); | ||
| AirState(); | ||
| explicit AirState(double const airDensity); |
There was a problem hiding this comment.
Why double and not Real64?
There was a problem hiding this comment.
That would be an oversight, will fix.
| int &OpeningProbStatus, | ||
| int &ClosingProbStatus); // function to perform calculations | ||
|
|
||
| bool openingProbability(EnergyPlusData &state, |
There was a problem hiding this comment.
What is the camelCase, snake_case algorithm here?
| IK.deallocate(); | ||
| AD.deallocate(); | ||
| AU.deallocate(); | ||
| void slvsky(const Array1D<Real64> &AU, // the upper triangle of [A] before and after factoring |
There was a problem hiding this comment.
Ah, Fortran six letter user defined names!
|
|
||
| // Object Data | ||
| EPVector<AirflowNetwork::AirflowNetworkReportVars> AirflowNetworkZnRpt; | ||
| std::unordered_map<std::string, std::string> UniqueAirflowNetworkSurfaceName; |
There was a problem hiding this comment.
Not a huge fan of using map in general, but could be a legitimate use. How many surfaces are typically in this structure? All surfaces in the model, i.e., hundreds or thousands? Only surfaces with openings? Also, if you are not mapping to anything, you can use unordered_set.
There was a problem hiding this comment.
I think this one gets used in VerifyUniqueInterObjectName? I believe that is used to make sure that no two AFN surfaces refer to the same heat transfer surface. That might be something that should be done in schema validation?
There was a problem hiding this comment.
Oh, and the number of elements in there is going to be on the order of the number of surfaces in the model.
There was a problem hiding this comment.
I don't know JSON schema very well, but I think this would be hard to do in validation. It's not a unique key situation, it's a uniqueness constraint on an arbitrary field. How would you even specify the level of the schema at which the field would have to be unique?
There was a problem hiding this comment.
Over in XSD land, one can definitely apply those sorts of constraints on fields. I don't know if you can require a field to be unique and also be from another set of elements. Maybe that's not possible with JSON schema? If it is it is probably complicated. It's pretty complicated with XSD, IIRC. In any case, I'm not sure why this needs a map when a vector of all the used names should be sufficient, but maybe I'm missing something.
There was a problem hiding this comment.
vector is fine too. (unorderd_)map/set is only better when you have many (> 20 say) elements to search through. Also, these kinds of searches should generally be done with surface indices rather than surface names, and for integers vector is 100% better even for many elements (or at least as many as you are likely to get in EnergyPlus).
| int HybridGlobalErrIndex = 0; | ||
| int HybridGlobalErrCount = 0; | ||
| int AFNNumOfExtOpenings = 0; // Total number of external openings in the model | ||
| int OpenNuminZone = 0; // Counts which opening this is in the zone, 1 or 2 | ||
|
|
||
| std::unordered_map<std::string, AirflowElement *> elements; |
There was a problem hiding this comment.
This bad girl I'm less sure about.
There was a problem hiding this comment.
This is the one I'm sure about. For historical reasons, models may have a very large number of elements (could be on the order of the number of surfaces), so there could be a lot of these (100s to 1000s). It is only used during setup to map an element name to the element that is needed.
| // Initialization | ||
| delzF = 0.0; | ||
| delzT = 0.0; | ||
| Interval = ActLh * OwnHeightFactor / NrInt; |
There was a problem hiding this comment.
auto &afnLinkageData = state.afn->AirflowNetworkLinkageData(il);
|
|
||
| lF = 1; | ||
| lT = 1; | ||
| if (From == 0) { |
There was a problem hiding this comment.
Yes, probably. This is F77++ in it's natural habitat, will need to go over all of this.
| // Helmut E. Feustel and Alison Rayner-Hooson, "COMIS Fundamentals," LBL-28560, | ||
| // Lawrence Berkeley National Laboratory, Berkeley, CA, May 1990 | ||
|
|
||
| // USE STATEMENTS: |
There was a problem hiding this comment.
I believe we are eliminating empty comment sections. Use statements are evil anyways.
| Array1D<Real64> Hfl(state.afn->NumOfLinksMultiZone); // Own height factor for large (slanted) openings | ||
| int Nl; // number of links | ||
|
|
||
| Array1D<Real64> DpF(2); |
There was a problem hiding this comment.
All of these would ideally be std::array so that you are not dynamically allocating them on every call. You could make it std::array<Real64, 3> if you don't want to deal with the 0-base thing right now. You can also leave as Array1D but make static so that it is allocated only once (note, this will make the function thread- and reentry- unsafe).
There was a problem hiding this comment.
I think this probably could be just the two numbers, it does kind of get looped over but in a very F77 way, there's a refactor needed.
|
|
||
| for (i = 1; i <= Nl; ++i) { | ||
| // Check surface tilt | ||
| if (i <= Nl - state.afn->NumOfLinksIntraZone) { // Revised by L.Gu, on 9 / 29 / 10 |
There was a problem hiding this comment.
auto &afnLinkageData = state.afn->AirflowNetworkLinkageData(i);
| // Functions | ||
|
|
||
| AirProperties::AirProperties(double const density) | ||
| // Using/Aliasing |
| { | ||
|
|
||
| // SUBROUTINE INFORMATION: | ||
| // AUTHOR Lixing Gu | ||
| // DATE WRITTEN Aug. 2003 | ||
| // DATE WRITTEN July 28, 2005 |
There was a problem hiding this comment.
How would one even know this?
| ReferenceConditions defaultReferenceConditions("Default"); // Defaulted conditions | ||
| bool conditionsAreDefaulted(true); // Conditions are defaulted? | ||
| CurrentModuleObject = "AirflowNetwork:MultiZone:ReferenceCrackConditions"; | ||
| auto instances = m_state.dataInputProcessing->inputProcessor->epJSON.find(CurrentModuleObject); |
| m_state.dataInputProcessing->inputProcessor->markObjectAsUsed(CurrentModuleObject, instance.key()); // Temporary workaround | ||
|
|
||
| std::string mixer_name = UtilityRoutines::MakeUPPERCase(fields.at("outdoor_air_mixer_name").get<std::string>()); | ||
| Real64 coeff{fields.at("air_mass_flow_coefficient_when_no_outdoor_air_flow_at_reference_conditions")}; |
There was a problem hiding this comment.
Really digging this field name.
| Real64 D{fields.at("hydraulic_diameter")}; | ||
| Real64 A{fields.at("cross_section_area")}; | ||
| Real64 e{0.0009}; | ||
| if (fields.find("surface_roughness") != fields.end()) { |
There was a problem hiding this comment.
Uhhh ... is this the most efficient way to do this? Isn't doing a find followed by an at doing two lookups for the same object? Wouldn't it be better to do
auto &f = fields.find("surface_roughness");
if (f != fields.end())
e = f.get<Real64>();
| hin = fields.at("inside_convection_coefficient").get<Real64>(); | ||
| } | ||
|
|
||
| DisSysCompDuctData(i).name = thisObjectName; // Name of duct effective leakage ratio component |
There was a problem hiding this comment.
You should know what I will way here.
| // or "ADJACENTENTHALPY" | ||
| if (!lAlphaBlanks(4)) MultizoneSurfaceData(i).VentControl = Alphas(4); | ||
| // Name of ventilation temperature control schedule | ||
| if (!lAlphaBlanks(5)) MultizoneSurfaceData(i).VentSchName = Alphas(5); |
| } | ||
| } | ||
| } | ||
| MultizoneSurfaceData(i).ModulateFactor = Numbers(2); // Limit Value on Multiplier for Modulating Venting Open Factor |
There was a problem hiding this comment.
That thing I always say.
| } | ||
| } | ||
| // Get data of polygonal surface | ||
| if (!lAlphaBlanks(8)) { |
| } | ||
| } | ||
| } else { | ||
| minHeight = min(m_state.dataSurface->Surface(MultizoneSurfaceData(i).SurfNum).Vertex(1).z, |
| Alphas(3) + " are zone nodes"); | ||
| ErrorsFound = true; | ||
| } | ||
| if (IntraZoneLinkageData(i).NodeN |
There was a problem hiding this comment.
I will deal with whatever all this is later.
| Alphas(3) + " are zone nodes"); | ||
| ErrorsFound = true; | ||
| } | ||
| if (IntraZoneLinkageData(i).NodeN |
There was a problem hiding this comment.
format the whole thing, not just the routine name.
| Alphas(3) + " are zone nodes"); | ||
| ErrorsFound = true; | ||
| } | ||
| if (IntraZoneLinkageData(i).NodeN |
There was a problem hiding this comment.
A pattern is beginning to emerge.
| Alphas(3) + " are zone nodes"); | ||
| ErrorsFound = true; | ||
| } | ||
| if (IntraZoneLinkageData(i).NodeN |
| } | ||
| Real64 pressure(101325.0); | ||
| if (fields.find("reference_barometric_pressure") != fields.end()) { // not required field, has default value | ||
| pressure = fields.at("reference_barometric_pressure").get<Real64>(); |
There was a problem hiding this comment.
There are new worker functions (#8738), would they not work here? or are more complicated than needed? I haven't used them yet so not sure.
getAlphaFieldValue(state, CurrentModuleObject, objectFields, objectSchemaProps, "alpha_field_name")
getRealFieldValue(state, CurrentModuleObject, objectFields, objectSchemaProps, "real_field_name")
e.g., thisMaterial.Thickness = ip->getRealFieldValue(objectFields, objectSchemaProps, "thickness");
There was a problem hiding this comment.
I totally forgot about that!! Looking at that code, it looks like it handles defaults in an inefficient way, but maybe I just don't understand what the epJSON default semantics are. It looks like you get a default if the field doesn't appear in the object (okay) and also if the field appears in the object but it has no value (is this a thing? Is this only a thing when you are translating from IDF?). Default finding also does automatic type conversions, e.g., if you expect a string and you find an integer it writes that integer to a string, etc. Feels like a lot is going on there. It's trying to be too robust to handle the possibility of poorly formed input. But it should work. We can try to lean out that code also.
There was a problem hiding this comment.
This was originally done before the worker functions were a thing, and this object was a little strange to being with in that originally had no required fields except for the name. We'll be addressing the defaults and all that in the epJSON work that @mbadams5 is starting.
There was a problem hiding this comment.
Sounds like a go to me. @jasondegraw I wasn't suggesting this change, just pointing out the resource.
| const AirState &propM, // Node 2 properties | ||
| std::array<Real64, 2> &F, // Airflow through the component [kg/s] | ||
| std::array<Real64, 2> &DF // Partial derivative: DF/DP | ||
| ); |
There was a problem hiding this comment.
Is this intentionally named in camel_case while GenericDuct is PascalCase?
There was a problem hiding this comment.
Yes, I haven't worked on GenericDuct and I've left it in the original form to remind me of that. When I do get to it, maybe later this year, that'll mean simulation diffs so I want to leave it as is for now.
| @@ -226,6 +226,26 @@ namespace AirflowNetwork { | |||
| int constexpr AirflowNetworkControlMultiADS(5); // Perform distribution system during system on time | |||
There was a problem hiding this comment.
Should this be an enum/enum class?
There was a problem hiding this comment.
It will be eventually.
|
|
||
| void clear_state() override | ||
| { | ||
| OccupantVentilationControl.deallocate(); |
There was a problem hiding this comment.
I think the entire guts here could be replaced with *this = Solver();. I'm a little surprised to see so many methods on this state class. Not saying it's wrong, that's just not something we've done as yet. We've been trying to thin out the state classes, but this could be part of the next evolution.
There was a problem hiding this comment.
I'm not sure I like the way that looks, but sure. Before the state refactor, I had broken out a solver object that was intended to do all the solving while the static data would reside elsewhere. This version contains all the data. Breaking some of that out again later would be fine but the separation of static and dynamic data is not fully complete. So I'd rather get it all gathered together so that more OOP patterns are possible and work on getting the static/dynamic thing sorted out on the fly.
| Height = state.dataAirflowNetwork->MultizoneSurfaceData(IL).Height; | ||
| Fact = state.dataAirflowNetwork->MultizoneSurfaceData(IL).OpenFactor; | ||
| Loc = (state.dataAirflowNetwork->AirflowNetworkLinkageData(IL).DetOpenNum - 1) * (NrInt + 2); | ||
| Width = state.afn->MultizoneSurfaceData(IL).Width; |
There was a problem hiding this comment.
This is just a lot more readable. 👍
There was a problem hiding this comment.
Yes, I've left the namespace as is because most access of these will be through the object now. There is still some repetition of AirflowNetwork around, but this PR is already so big I'd rather leave that for another day.
|
I've cleaned out some of the poor usage of So as far as I know, I've responded to most of the comments thus far and either made changes or added something to the to-do list. Let me know if I missed anything. |
|
I've finally gotten the warnings taken care of, had to learn how to do that with CMake. There's a comment or two in the CMake files about revisiting some of the warnings that are turned off. I think the time to do that needs to be sooner rather than later. I am seeing a lot of warnings from the Visual Studio IDE. I won't try to do a location-by-location walkthrough at this point given all the previous comments, but the main points are that all of the AirflowNetwork data are collected into a single object and the related functions have been made into member functions for that objects. This simplifies and unifies access to the feature through the object. I have tried to reduce duplication of naming (less AirflowNetwork::AirflowNetworkSomething) and access via the object reduces the number of times the namespace even needs to be worried about. I have also cleaned up some I'd like to get this merged soon to clear the decks for @lgu1234 to do the duct sizing work. |
|
I agree this should go in soon. CI is all green, so while there are certainly things that we could continue to find to work on with this, I'm good getting this in to free up other efforts. Anyone else have any comments here? |
|
👍 that was plenty of time to open for comments, this is going in. Thanks @jasondegraw ! |
Pull request overview
This PR refactors AirflowNetwork and continues work that I started several years ago to make AFN more object oriented. There are still some things to fix, but this is enough for the work I am currently doing. A single solver object is the point of entry for all of the solver-type functionality and resolves some ambiguity that had been unintentionally introduced. It has uncovered a number of things that will need fixing, will follow up on those things later.
Diffs are expected for any file that generates warnings or errors from AirflowNetwork because of some name changes that change how the messages are labeled.
Will add some walk-through comments later.
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.