Skip to content

Conversation

@kldjonge
Copy link
Contributor

No description provided.

@kldjonge kldjonge marked this pull request as ready for review April 29, 2025 09:57
@kldjonge
Copy link
Contributor Author

@jelgerjansen, can you please update the base branch to master to clean out the "files changed" tab? And if that doesn't work try switchting the base branch for a second and then back? (https://github.com/orgs/community/discussions/16351)

@jelgerjansen jelgerjansen changed the base branch from issue1338_verifyVerticalHeights to master July 10, 2025 09:59
@jelgerjansen jelgerjansen changed the base branch from master to issue1338_verifyVerticalHeights July 10, 2025 10:01
@jelgerjansen
Copy link
Contributor

@kldjonge I updated the branch open-ideas:issue1338_verifyVerticalHeights this morning to be up-to-date with the master branch, and changing the base to the master branch still leaves a lot of additional files in the "files changed" tab.
I checked and noticed that your branch still contains files that are not in the IDEAS master branch (e.g. IDEAS/Resources/src/convertEPW/doc/...). Not sure how this happened, but you'll have to fix this on your end to clean out the "files changed".

kldjonge added 9 commits July 10, 2025 14:32
Getting the derivatives of the spline only needs to be done once but was done each time the function was called.
Updated hFloor for PPD12 to account for floor heights. And set n50 parameter to use the inherited value.
… where necessary

Updates to include the vertical height check including floor thicknesses.

incInt where neccecarry in partial surface and internal wall, this is needed for when inc is set a custom value
This also tests the vertical height check asserts
included CO2 in the medium so I could check if the background CO2 of 400ppm actually entered the building. hFloor adapted in line with the vertical height check
@kldjonge
Copy link
Contributor Author

kldjonge commented Jul 10, 2025

@jelgerjansen, I managed to fix it!

Added hVertical/2 to the error message so that the message also worked for errors when walls are vertical (e.g. split levels)
The n50_computed parameter is now always set to n50_int, removing the conditional logic since n50_int itself allready has the same conditional logic. This simplifies the parameter definition and should not lead to any changes in the reference results.
Updated the description string for the hRelSurfBot_a and hRelSurfBot_b parameters in PartialSurface and InternalWall to make them consistent.
Copy link
Contributor

@jelgerjansen jelgerjansen left a comment

Choose a reason for hiding this comment

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

@kldjonge apart from the comments, don't forget to merge the latest master branch first to avoid conflicts (then I can also change the base branch to master)

@kldjonge
Copy link
Contributor Author

@kldjonge apart from the comments, don't forget to merge the latest master branch first to avoid conflicts (then I can also change the base branch to master)

@jelgerjansen, I merged the master (no conflicts). Can you maybe change the base branch allready or also merge the master in https://github.com/open-ideas/IDEAS/tree/issue1338_verifyVerticalHeights ?

@kldjonge
Copy link
Contributor Author

@jelgerjansen , potentially 55a535a, will shift the reference results for models with airflow. In IDEAS.Examples.TwinHouses.BuildingO5_Exp1_1Port, I allready checked the 'comparison' variables and that looks good.

@kldjonge
Copy link
Contributor Author

Below you can find the reference results that changed. Can you verify whether these make sense? IDEAS_Buildings_Components_Examples_TwoStoreyBoxes IDEAS_Examples_PPD12_VentilationRBC

Both make sense. Both will probably change a bit again.

Added a reference to GitHub issue open-ideas#1417 in the changelog for the InternalWall component to provide additional context for recent changes.
@kldjonge kldjonge requested a review from jelgerjansen August 19, 2025 13:17
kldjonge and others added 7 commits August 19, 2025 16:10
I suggest to add component-level output variables with consistent naming and positive values for the same flow direction, already much easier to interpret for the user although further improvements could be made later.
The function sin(pi) or pi(0) is approximate and did not lead to a clean dh=0 for ceilings or roof, which is a bit annoying.
Actually not nececarry to have two, simplifies without loss in accuracy
@jelgerjansen
Copy link
Contributor

jelgerjansen commented Aug 19, 2025

@kldjonge new reference results.
Please merge my latest changes to your branch. If the unit tests pass, we can merge.

IDEAS_Buildings_Components_Examples_TwoStoreyBoxes image IDEAS_Examples_PPD12_VentilationRBC

@kldjonge
Copy link
Contributor Author

@jelgerjansen, thanks for the support and reviews on this big one! Appreciate it!

@jelgerjansen jelgerjansen merged commit 2080e26 into open-ideas:master Aug 20, 2025
2 checks passed
@jelgerjansen jelgerjansen deleted the hfloorcheck branch August 20, 2025 12:25
@jelgerjansen jelgerjansen linked an issue Aug 20, 2025 that may be closed by this pull request
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.

Support including floor thinkness for interzonalairflowtype.TwoPort verify whether hZone is filled in

3 participants