Skip to content
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

Stack effect airflow #1296

Closed
wants to merge 119 commits into from
Closed

Stack effect airflow #1296

wants to merge 119 commits into from

Conversation

Mathadon
Copy link
Member

@Mathadon Mathadon commented Aug 30, 2022

For #1244

kldjonge and others added 30 commits December 11, 2020 12:12
Updated master from IDEAS Master
Debugging partial surface, new implementation of stack columns because conditional connections to col models did not work as expected.
Got rid of the unnecessary initial equations
New implementation for the density columns to avoid error due to conditional statements in the column heights
Changed error in input of door component.
… and flow element(s)

Assign height difference between meteorological pressure measurements and flow element(s). Also, by default corrects for the windspeed using this height when the TwoPort implementation is used.
cosmetic change
option added to get better default for roofs. This needs to be fine-tuned based on available data.
@jelgerjansen
Copy link
Contributor

@Mathadon @kldjonge I ran IDEAS.Examples.TwinHouses.BuildingO5_Exp1_1Port.mo again. The results are already better, but still errors compared to the CONTAM model in the order of 1e-2. Could this be due to the default wind pressure computation?
image
image

Furthermore, the newly add unit test for BuildingO5_Exp1_2Port throws an error due to the absence of resDoor (which is needed to calculate comparison_W40): in this wall, useDooOpe is true, due to which useResDoor is false (see InternalWall model implementation)

@Mathadon
Copy link
Member Author

Mathadon commented Sep 7, 2023

Ah, now I remember something else: the 1 port door configuration contains a medium column, which I have removed in my pull request. This indeed seems wrong and was not included in the contam reference results either. So it should probably be removed before rerunning the reference results. @kldjonge ?

@kldjonge
Copy link
Contributor

True. For this branch/unit test it should not be included. (We can discuss maybe adding density columns in a later stage just for the larger openings, but not for now)

@jelgerjansen
Copy link
Contributor

@Mathadon @kldjonge could you explicitly tell me what I should remove in which models? Then I can rerun the unit tests after applying the changes (and such that we can hopefully move on to the other PRs after that ;) )

@kldjonge
Copy link
Contributor

@jelgerjansen, apparently having branches/pull request with exactly the same name causes issues. At least in the tool I use, I can't load this pull request so being very specific is hard.
If I recall correctly, the column we are talking about is in the internal wall component and was added between the door component and the connectors.

@jelgerjansen
Copy link
Contributor

@Mathadon @kldjonge removing the density columns between the door, i.e. going from this:
image
to this:
image
gives the same results as plotted here: #1296 (comment)...

Removing the density columns therefore didn't affect any of the reference results...

@jelgerjansen
Copy link
Contributor

jelgerjansen commented Sep 19, 2023

Thank you @kldjonge, this indeed fixes the problem; the reference results are now exactly the same as those of the master branch:
image
image

@Mathadon can you comment on the specific implementation of Cs as noted by Klaas above?

Furthermore, before pushing the new changes, I would like to know what to do with the unit test for IDEAS.Examples.TwinHouses.Building05_Exp1_2Port, which threw an error (see #1296 (comment))

@Mathadon
Copy link
Member Author

@jelgerjansen no, I'm not too familiar with that model.

With respect to the error, I'd remove the offending variables in the .mos file for now :)

Mathadon added a commit to Mathadon/IDEAS that referenced this pull request Sep 19, 2023
@jelgerjansen
Copy link
Contributor

Due to the change in calculation of Cs, all previously changed reference results are also different of course. Give me some time to reset the reference results again to the 'original' ones, such that we can compare again with those instead of the reference results with the 'wrong' Cs values.

@Mathadon
Copy link
Member Author

okay, sounds like a good extra check!

@jelgerjansen
Copy link
Contributor

Running the unit tests using the updated Cs calculation and comparing to the 'original' reference results leads to lower deviations between the new and old reference results. An overview of the changes can be found below:
IDEAS Buildings Examples ScreenComparison
IDEAS Buildings Examples ZoneExample
IDEAS_Examples_PPD12_VentilationRBC

I think we can therefore continue and merge these changes in #1322?

The reference results of TwinHouses are, in contrast to the first unit test that I ran, not changed.

@kldjonge
Copy link
Contributor

I think we can move on to the next branch indeed. Great!

@jelgerjansen
Copy link
Contributor

The changes of this PR are included in #1327. Further development of the stack airflow effect will take place in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants