-
Notifications
You must be signed in to change notification settings - Fork 39
Add interior mass nodes #131
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
Conversation
|
@s2t2 I can't assign reviewers. Please see my above responses. |
|
@ecosang awesome! I will aim to review this week. |
s2t2
left a comment
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.
Hi @ecosang I have conducted a preliminary review with some comments that I will release now, however I still need to spend some more time to complete my review, which I aim to do next Tuesday.
| conductivity=0.05, heat_capacity=500.0, density=3000.0 | ||
| ) | ||
| interior_mass_properties = building.MaterialProperties( | ||
| conductivity=0.5, heat_capacity=1000.0, density=2000.0 |
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.
for the MaterialProperties class, it would be great if we could add a value lookup table in the docs (like we did for the RadiationProperties).
I know the MaterialProperties is not a new class, but I was wondering if you have any insight into what some default values would be for common materials.
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.
The original material properties are designed by Google team, and I made radiative property with you.. so I will find out default values and then make them consistent.
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.
@s2t2 Hi Michael, I will explain during the meeting, but this is not a straightforward process. The main reason is sbsim uses the same properties for all wall nodes, which means if there are multiple layers of insulation, it is hard to define them. Another case is that the indoor air property is somewhat unrealistic. The original implementation has the indoor as somewhat higher mass to capture interior_mass property to some extent. Also, indoor air heat transfer is driven by mixing/convection, while sbsim models it as a conduction process, which is not true. I think this is because the sbsim is targeting data-driven calibration instead of specifying material properties. So, if we capture the behavior with some number to some extent from the data-driven calibration, I think it is fine. To sum, the material properties are not really physical numbers, but lumped values to capture thermal behaviors.
Radiative one is different. We are dealing with the material directly (surfaces), so we can define it still in sbsim framework.
The current default values are reasonable in my opinion.
"""
inside_air_properties = building.MaterialProperties(
conductivity=50.0, heat_capacity=700.0, density=1.2
)
inside_wall_properties = building.MaterialProperties(
conductivity=2.0, heat_capacity=1000.0, density=1800.0
)
building_exterior_properties = building.MaterialProperties(
conductivity=0.05, heat_capacity=1000.0, density=3000.0
)
"""
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.
@ecosang thanks for the explanation.
Quick follow-up question: If I am understanding correctly (that some of the currently used material property values are unrealistic because they are compensating for a lack of de-tangling between inside air vs inside mass), would we then want to use different default values depending on whether or not the interior mass mode is on? For example we can choose more realistic values if interior mass mode is on.
Let me know if this suggestion is reasonable and helpful, or way off base 😅
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.
@s2t2 Yes, your interpretation is correct.
For the exterior and interior walls, I think I can do some calculations and provide some default values.
For example, a typical wall composition's R value is 2.5 (m2K/W) and its thckness is 0.13m, so the averaged conductivity is 0.052 W/mK. But, for the capacity and density, I need to find all the materials information and wall composition (thickness) and need to calculate the weighted average value.
For the interior wall, I can do something similar.
Which is doable, but the results are similar to the current default values.
The air node is the problem. When using interior mass, yes, we can put a realistic number. Without interior_mass, we cannot. We can use some numbers that incorporate interior_mass+air information altogether, like the current implementation.
After all, the sbsim's core work is data-driven model, and the calibration of material properties is essential.
Based on the floor plan and measured data, the material properties need to be tuned. So, the default values are not very useful in my view. On the other hand, the radiative heat transfer is slightly different. Their impacts might be minimal in the simulation, and using default values are more robust as they are a direct material property values.
At this point, I have several things to do, so I would like to leave it as it is, and once I have more confidence in the interior_mass model, I would like to have default values. (Another thing is, we don't have inter-air heat transfer through mixing in sbsim. sbsim overcomes this by having high conductivity number for the air, but the air's conductivity is really small. Instead, the inter-air heat transfer is achieved via air mixing, not conduction. With the interior_mass model, if I put a realistic number for air (i.e., low conductivity), then the inter-air heat transfer may disappear. So we need to think of how to model this..)
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.
OK, thank you for the explanation. Sounds good.
Let me know if you plan on making any other updates to this PR, or if you think it is ready for final review and merge.
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.
@s2t2 I am good with this PR. Other updates come in separate PRs.
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.
Hi @ecosang I have completed my initial review. I also shared some comments from Gemini.
This looks great in general!
The main change I would like to see is to refactor some of the shared setup steps for the tests (which I believe you were starting to do).
Also as previously mentioned, it would be great to add more documentation about common material property values. It would be nice to also include some comments about why we are using certain values in our tests.
Feel free to mark any comments / conversations as resolved if they don't require further action.
|
@s2t2 I resolved all your comments in my opinion, except for the default values.. Do you have any other suggestions? It is not urgent, but I want to make a new branch based on here to continue exterior radiative heat transfer modeling. |
s2t2
left a comment
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.
LGTM 🙌 🚀
After adding this feature, you can now see a greater temperature gradient due to the interior thermal mass.

I've implemented it for the iterative approach and the tensorized approach. Test cases added, and the formulations are included in the docstrings.
Finally, the calculation is validated via tf_simulator_test.py by comparing with the iterative approach. One thing I found is that when the convergence threshold is too small (1e-4), the convergence takes lots of time as the interior mass's temperature change is very slow.
Also, some minor fixes for interior radiative heat exchange, such as naming convention (capital letters to small letters).
Hardcoded expected results are converted to .csv file in test cases.