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

feat(at): Added apparent temperature(AT). #44

Merged
merged 2 commits into from
Oct 30, 2019
Merged

feat(at): Added apparent temperature(AT). #44

merged 2 commits into from
Oct 30, 2019

Conversation

janeliutw
Copy link
Contributor

@janeliutw janeliutw commented Oct 26, 2019

issue #33
This PR only covers adding the apparent temperature. I also added two unit tests to cover the functions.
I was confused by the "Radiative-effective temperature" in the effective temperature legacy code, not sure which value -- TE or TRE, should I return when I re-write the method. Hence I didn't complete the effective temperature part. But I am happy to work on it if anyone can help me understand this part.

Copy link
Member

@chriswmackey chriswmackey left a comment

Choose a reason for hiding this comment

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

This is a great PR, @janeliutw . Thank you for putting it together.

The code is good enough to merge in so I'm approving it but I had a few minor comments that are all about styling and should be pretty quick to implement. If you are able to address the comments before we merge it in, it would be a big help.

Also, I realize the ET stuff is more complex and we can break it out into another issue and another PR.

ladybug_comfort/apparent.py Outdated Show resolved Hide resolved
ladybug_comfort/apparent.py Outdated Show resolved Hide resolved
ladybug_comfort/apparent.py Outdated Show resolved Hide resolved
ladybug_comfort/apparent.py Outdated Show resolved Hide resolved
tests/apparent_test.py Outdated Show resolved Hide resolved
ladybug_comfort/apparent.py Outdated Show resolved Hide resolved
@chriswmackey chriswmackey added hacktoberfest new development For issues that require new code labels Oct 28, 2019
A minor update to improve code efficiency.
@janeliutw
Copy link
Contributor Author

@chriswmackey Thank you for the code review. I have updated the files according to your comments. Let me know if you have more concerns or comments, I am happy to adjust my code.

@chriswmackey
Copy link
Member

This looks great, @janeliutw . Thank your for addressing all of the comments. Let's merge this in and thanks again for putting together a good Hacktoberfest contribution!

@chriswmackey chriswmackey merged commit 88063e5 into ladybug-tools:master Oct 30, 2019
@chriswmackey
Copy link
Member

And welcome to the contributor's list!

@ladybugbot
Copy link
Contributor

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@lauleh
Copy link

lauleh commented Nov 11, 2019

Good evening @janeliutw
My name is Lauleh and I am reaching out from Ladybug Tools.
I want to personally thank you for making contributions to our repositories during the Hacktoberfest 2019.
If you can kindly take a moment to e-mail me ([email protected]) your mailing address, I would like to send you a Ladybug Tools sticker. Thank you so much!

Best,

--
Lauleh Aslani | Administrative Assistant
Ladybug Tools
www.ladybug.tools | [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants