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

Fix incorrect MRT calculation for some epws #233

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

Tom-Kingstone
Copy link
Contributor

NOTE: Depends on

Issues addressed by this PR

Closes #232

Changes made in accordance with linked issue.

If sun exposure is null, then use sky exposure instead when calculating shaded and unshaded MRT.

Test files

Verify that MRT is being calculated properly (@tg359 has an epw that caused this issue before)

Changelog

Additional comments

@Tom-Kingstone Tom-Kingstone added the type:bug Error or unexpected behaviour label Aug 1, 2024
@Tom-Kingstone Tom-Kingstone requested a review from tg359 August 1, 2024 13:55
@Tom-Kingstone Tom-Kingstone self-assigned this Aug 1, 2024
@Tom-Kingstone
Copy link
Contributor Author

@BHoMBot check code
@BHoMBot check compliance

Copy link

bhombot-ci bot commented Aug 1, 2024

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

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

There are 14 requests in the queue ahead of you.

@Tom-Kingstone
Copy link
Contributor Author

@BHoMBot check core

Copy link

bhombot-ci bot commented Aug 1, 2024

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

  • check core

There are 21 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.

Based on a functionality check with the weatherfile that was showing issues (and another which wasn't) nothing seems to have broken and the changes make sense, so I approve them.

@Tom-Kingstone
Copy link
Contributor Author

@BHoMBot check versioning
@BHoMBot check installer

Copy link

bhombot-ci bot commented Aug 1, 2024

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

  • check versioning
  • check installer

@Tom-Kingstone
Copy link
Contributor Author

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

Copy link

bhombot-ci bot commented Aug 1, 2024

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

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

@Tom-Kingstone
Copy link
Contributor Author

@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented Aug 2, 2024

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

  • check ready-to-merge

@jamesramsden-bh jamesramsden-bh merged commit 50c09bf into develop Aug 2, 2024
13 checks passed
@jamesramsden-bh jamesramsden-bh deleted the LadybugTools_Toolkit-#232-MRTFix branch August 2, 2024 08:24
@BHoMBot BHoMBot mentioned this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NaN handling for sun exposure resulting in incorrect MRT calculation
3 participants