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

Issue1433 shadow model #1464

Merged
merged 23 commits into from
Jan 15, 2024
Merged

Issue1433 shadow model #1464

merged 23 commits into from
Jan 15, 2024

Conversation

Jun-Jiang-92
Copy link
Contributor

In this issue, a new feature named "shadow model" is created. This model is built in the new package "AixLib.ThermalZones.HighOrder.Components.Shadow" and is integrated into the "AixLib.ThermalZones.HighOrder.Components.Walls.Wall" model.

By using shadow model, the impact of the shadow, generated by the horizontal shield above the window, will be considered. The solar heat gain through the window glazing will be reduced according to the shield length and shadow length during the simulaiton.

To activate the shadow model, it is necessary to activate the "withSunblind" in the tab "Window". And then, it is possible to activate "withShield" and input the parameters for the model.

@Jun-Jiang-92 Jun-Jiang-92 added the New Model A new model is developed in this branch label Sep 10, 2023
@Jun-Jiang-92 Jun-Jiang-92 self-assigned this Sep 10, 2023
@Jun-Jiang-92 Jun-Jiang-92 linked an issue Sep 10, 2023 that may be closed by this pull request
@ebc-aixlib-bot
Copy link
Contributor

Errors in regression test. Compare the results on the following page
https://ebc.pages.rwth-aachen.de/EBC_all/github_ci/AixLib/issue1433_shadow_model/plots

Copy link
Contributor

@FWuellhorst FWuellhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jun-Jiang-92 Thanks for this feature. I added minor styleguide requests. We have to discuss in person on why the weaBus is necessary and, if it is, how to best implement it.

AixLib/ThermalZones/HighOrder/Components/Walls/Wall.mo Outdated Show resolved Hide resolved
@ebc-aixlib-bot
Copy link
Contributor

Errors in regression test. Compare the results on the following page
https://ebc.pages.rwth-aachen.de/EBC_all/github_ci/AixLib/issue1433_shadow_model/plots

@ebc-aixlib-bot
Copy link
Contributor

Errors in regression test. Compare the results on the following page
https://ebc.pages.rwth-aachen.de/EBC_all/github_ci/AixLib/issue1433_shadow_model/plots

@Jun-Jiang-92
Copy link
Contributor Author

@FWuellhorst I updated the codes as we discussed and renewed the pull request. If there is anything further that should be done here, please feel free to inform me.

@FWuellhorst
Copy link
Contributor

Thanks @Jun-Jiang-92 ! I updated the branch with the latest dev, and we have to check which regression tests are failing.

@ebc-aixlib-bot
Copy link
Contributor

Errors in regression test. Compare the results on the following page
https://ebc.pages.rwth-aachen.de/EBC_all/github_ci/AixLib/issue1433_shadow_model/plots

@FWuellhorst
Copy link
Contributor

@Jun-Jiang-92 : I made minor adjustments in your code. Everything looks good, except that a documentation is currently missing. Please indicate with 1-3 sentences what the models are used for and central assumptions. Also, add yourself to the revision section as done in other models and link #1433 .
Afterwards, you are free to merge! Nice work :)

@FWuellhorst
Copy link
Contributor

FWuellhorst commented Dec 18, 2023

Regarding the failing tests: I don't get how they are failing, as the models did not change. If the current dev pipeline passes, I will look into this again. The only thing failing are the ASHREA cases:

Case950_AnnualCoolingLoad
Case220_AnnualHeatingLoad
Case210_AnnualHeatingLoad
Case250_AnnualHeatingLoad
Case430_AnnualHeatingLoad
Case400_AnnualHeatingLoad
Case410_AnnualHeatingLoad
Case420_AnnualHeatingLoad
Case800_AnnualHeatingLoad

The error is minimal, and other test cases run just fine, with no clear difference between e.g. Case200 and Case210 regarding wall / window models:
image

@ebc-aixlib-bot
Copy link
Contributor

Errors in regression test. Compare the results on the following page
https://ebc.pages.rwth-aachen.de/EBC_all/github_ci/AixLib/issue1433_shadow_model/plots

@Jun-Jiang-92
Copy link
Contributor Author

@FWuellhorst Thank you for your adjustments and checking failures.
I already added the documentation and revision info to the models. Since the test is still not able to pass, it is not possible to merge the changes into the branch directly. Is that right?
Would you like to show me how to merge it in this situation if you have any time?

@DaJansenGit
Copy link
Member

Is my review still required here? Since @FWuellhorst is already in the review process, I don't think so, right?

@FWuellhorst
Copy link
Contributor

@DaJansenGit No, your review is not required anymore.
@Jun-Jiang-92 @DaJansenGit : I don't get why the regression tests fail, as the models do not use the shadow model. The errors are close to zero, an only certain variables are affected. I would opt to remove the existing regression files (.txt-files) to re-generate them by CI. Then, we can proceed and merge.

@ebc-aixlib-bot
Copy link
Contributor

Errors in regression test. Compare the results on the following page
https://ebc.pages.rwth-aachen.de/EBC_all/github_ci/AixLib/issue1433_shadow_model/plots

Copy link
Contributor

@FWuellhorst FWuellhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jun-Jiang-92 : Thnaks for the cool feature and the work associated with this! The CI passes now, and the changes look good. Feel free and click on merge :)
Edit: Wait for the stage to pass.

@Jun-Jiang-92 Jun-Jiang-92 merged commit e6bcdaa into development Jan 15, 2024
1 check was pending
@Jun-Jiang-92 Jun-Jiang-92 deleted the issue1433_shadow_model branch January 15, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Model A new model is developed in this branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add shadow model to Wall
4 participants