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

Add internal mass to ground zone simulations #195

Merged

Conversation

Erida-Bendo
Copy link
Contributor

Add InternalMass to ground zone creation part that feeds into SimulationResult process to get surface temperatures to replicate more accurate ground conditions.

@Erida-Bendo Erida-Bendo requested a review from tg359 May 22, 2024 13:23
@Erida-Bendo Erida-Bendo linked an issue May 22, 2024 that may be closed by this pull request
@Tom-Kingstone Tom-Kingstone added the type:feature New capability or enhancement label May 23, 2024
Copy link
Contributor

@Tom-Kingstone Tom-Kingstone left a comment

Choose a reason for hiding this comment

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

Please remove the .idea folder from this PR (this can be done by reverting the most recent commit).
Otherwise code-wise the changes look good. Will test these changes myself when I have a fresh install of pollination.

@Tom-Kingstone Tom-Kingstone changed the title Ladybug tools toolkit #188 add internal mass ground zone s im Add internal mass to ground zone simulations May 23, 2024
remove data folder saved from PyCharm
@Tom-Kingstone
Copy link
Contributor

@BHoMBot check core
@BHoMBot check compliance

Copy link

bhombot-ci bot commented May 24, 2024

@Tom-Kingstone to confirm, the following actions are now queued:

  • check core
  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

Copy link
Contributor

@Tom-Kingstone Tom-Kingstone left a comment

Choose a reason for hiding this comment

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

Tested and can confirm that the changes do not break simulation models, though I can't comment on how valid the simulation results are. Await approving review from @tg359 before merge

@Tom-Kingstone
Copy link
Contributor

@BHoMBot check installer
@BHoMBot check versioning

Copy link

bhombot-ci bot commented May 24, 2024

@Tom-Kingstone to confirm, the following actions are now queued:

  • check installer
  • check versioning

@Tom-Kingstone
Copy link
Contributor

@BHoMBot check null-handling
@BHoMBot check serialisation
@BHoMBot check unit-tests

Copy link

bhombot-ci bot commented May 30, 2024

@Tom-Kingstone to confirm, the following actions are now queued:

  • check null-handling
  • check serialisation
  • check unit-tests

There are 18 requests in the queue ahead of you.

Copy link
Contributor

@tg359 tg359 left a comment

Choose a reason for hiding this comment

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

Approved on the basis that the process hasn't broken and that inclusion of internal mass would only increase the accuracy of the simulation.

In the next testing sprint the unit tests downstream of this code will need to be updated (https://github.com/BHoM/LadybugTools_Toolkit/tree/develop/LadybugTools_Engine/Python/tests/test_external_comfort) to ensure that they too pass, as values will change from what the tests were written against, so those tests will likely fail until update are made.

@Tom-Kingstone
Copy link
Contributor

@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented May 30, 2024

@Tom-Kingstone to confirm, the following actions are now queued:

  • check ready-to-merge

@jamesramsden-bh jamesramsden-bh merged commit b471b67 into develop May 30, 2024
13 checks passed
@jamesramsden-bh jamesramsden-bh deleted the LadybugTools_Toolkit-#188-AddInternalMassGroundZoneSIm branch May 30, 2024 12:53
@jamesramsden-bh
Copy link
Contributor

Tom and I have reviewed unit tests, and no tests appear to be affected by these changes (either directly or downstream). This means that all unit tests are still passing, but also that there is probably a gap in the unit tests! I will add an issue to provide an additional test.

@bhombot-ci bhombot-ci bot mentioned this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add internal mass to ground zone simulation
4 participants