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

Issue1080 ROM soil temperature and interzonal heat exchange #1546

Open
wants to merge 55 commits into
base: main
Choose a base branch
from

Conversation

PGorzalka
Copy link

@PGorzalka PGorzalka commented Nov 17, 2024

Finally, I was able to finish the work on issue #1080
@DaJansenGit & @FWuellhorst thanks for your support!

Changes mainly encompass the work mentioned in this paper:

  • building up on changes already implemented via IBPSA merge, some adaptations for non-constant soil temperature
    • Option selector added to AixLib.BoundaryConditions
    • Options added to ZoneBaseRecord
    • Replaced TSoil constant in ThermalZone by option selector
  • Interzonal heat exchange with new FiveElements RC model
    • Integration of that model into ThermalZone
    • Interzonal connection of zones in Multizone. For OM compatibility, I had to add an additional NzSplitter model and parameters to make sure arrays are initialized without errors.
    • Added ASHRAE test case 960

Additional minor stuff:

  • made some parts of the ThermalZone model replaceable, so boundary conditions can be adapted more easily
  • two missing eachs in AixLib.Fluid.Pools.BaseClasses.AirFlowMoistureToROM caused OM to fail -> fixed

Missing:

  • Test Case 960 was not actually tested against standard results because the weather file provided (AixLib\Resources\weatherdata\ASHRAE140.mos) does not contain the weather data provided by the standard.

See also PR 785 in TEASER

Looking forward to your feedback on the changes!

PGorzalka and others added 19 commits April 20, 2023 11:07
…VDI6007 calculators. Made some elements in ThermalZone replaceable for future implementation of e.g. urban shading.
…n GroundTemperature/GroundTemperatureOptions.mo
FiveElements: bug fixed and documentation updated.
done:
- ROM RC module including pass-through
- connection between zones via additional NzSplitter.mo
…delica

left to do: remove redundant heatFlowSensor
…d-zone-borders

Issue1080 rom soil and zone borders
@FWuellhorst
Copy link
Contributor

@PGorzalka , nice, thanks for adding this nice feature to AixLib! We have an extensive CI which does a lot of groundwork of the review, but it only runs for internal branches. Thus, I added an internal branch based on your fork. Now, we see if naming guidelines and regression tests all run. Depending on the changes, you either can work in your fork and merge your fork into the internal branch, or we could add you as a developer so you can work directly in AixLib @DaJansenGit ?

@FWuellhorst FWuellhorst mentioned this pull request Nov 18, 2024
@DaJansenGit
Copy link
Member

DaJansenGit commented Nov 18, 2024

@PGorzalka thanks for contributing!
@FWuellhorst Lets see what the CI outputs. If there are bigger changes that need to be addressed, I would add Philip as developer. Thanks for creating the CI branch!

DaJansenGit and others added 2 commits November 18, 2024 06:49
…_issue1080_ROM-soil-and-zone-borders

Corrected HTML Code in branch correct_HTML_PGorzalka_issue1080_ROM-soil-and-zone-borders
@FWuellhorst
Copy link
Contributor

@PGorzalka : You can find check and naming errors here:
https://rwth-ebc.github.io/AixLib/PGorzalka_issue1080_ROM-soil-and-zone-borders/index.html
With regard to the naming violations, please just address new code lines/files you added.

@DaJansenGit
Copy link
Member

Ok, I never used something other than four elements. But, as this is the current practice, I would just add nIze=1 and the default values to the TEASER export, and specify it in the BaseRecord to be backwards compatible. image

On the long run, I think the record would benefit by using groups or tabs to shows users the different elements, use conditions, etc. I always disliked the record as a sort of global parameter object with no structure, and new (cool) features like this further increase complexity. But we should tackle this in a separate issue @DaJansenGit, or? Should be fast, as it is purely graphical. We could maybe add Booleans to disable the parameter view and avoid the A > 0 cases in the ROM, but that would not be compatible and require IBPSA changes, so no :D

I agree with the proposed solution to adjust the TEASER export and BaseRecord, so its backwards compatible. I did the changes in the CI branch and if CI passes I will merge it into this branch. In TEASER @PGorzalka already did add the changes to the export, so we should be fine.
Furthermore, I really like the idea of adding some dialog section to increase readability of the record.

@FWuellhorst
Copy link
Contributor

@DaJansenGit @PGorzalka : I updated the CI in PR #1554 to Dymola2024x. I would wait to merge this depending on your timeline. Maybe some regression results would need updating if they even exist yet. If they don't exist yet, this PR should not affect this PR.

@PGorzalka
Copy link
Author

@FWuellhorst @DaJansenGit I added the example and the respective script. I hope I did it correctly. But I honestly don't know how to handle the reference results. The explanation here seems to be outdated and BuildingsPy no longer used - at least the mentioned runUnitTests.py script is no longer presented in the AixLib repository?!

@FWuellhorst
Copy link
Contributor

@PGorzalka : mhm, this seems to be outdated indeed. Our CI pipeline generates the results for you.
This PR comment explains the new process: #1548 (comment) but is not in this PR as the CI does not run for forks. Lets see if the scripts work!

@FWuellhorst
Copy link
Contributor

The regression plots are displayed here: https://rwth-ebc.github.io/AixLib/PGorzalka_issue1080_ROM-soil-and-zone-borders/charts/ThermalZones/AixLib_ThermalZones_ReducedOrder_Examples_MultizoneInterzonalsFixedHeater.html
Are the fluctuations in zone 1 intended? I had to change to cvode solver, as radau gets timeouts in CI.

Some other results seem to have changed translation statistics. Without interzonal exchange, the change in soil temperature model could cause this, or?

@PGorzalka
Copy link
Author

PGorzalka commented Dec 11, 2024

Thank you for checking the results!

Regarding fluctuations: this is indeed intended. Zone 1 is an unheated attic with high infiltration.

Regarding translation: yes, this could be caused by the soil temperature model. Additionally, the FiveElement model has replaced the FourElement in the Multizone.

@FWuellhorst
Copy link
Contributor

Ok, it seems to only affect this model:

*** Warning: AixLib.ThermalZones.ReducedOrder.Examples.MultizoneMoistAirCO2: Translation statistics for simulation changed for linear, but results are unchanged.
 Old = 4, 0, 4, 0, 4, 0, 4, 0, 4, 0
 New = 3, 0, 3, 0, 3, 0, 3, 0, 3, 0

I will update these statistics, as smaller is better. Then you can update your fork with the main and the aixlib-internal fork branch.

@PGorzalka
Copy link
Author

Thank you! I merged the changes back into this branch. The CI branch still seems to have some issues though?!

@FWuellhorst
Copy link
Contributor

@PGorzalka it seems that your model takes more than 300 s to solve in CI, where multiple models run in parallel. Running it locally in docker on a single core, it works fine. 4 processors work fine as well, but I guess this is the problem, because various models get timeouts which worked fine before.
So, my idea would be to decrease the simulation time to maybe 3-4 days? As long as it is a phase with interzonal exchange, it should be sufficient for regression test purposes.

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