Refining Sunpath #32
Refining Sunpath #32
Conversation
These changes have mainly been directed towards improving the altitude and azimuth calculation.
|
@devngc thank you for the PR! This has been a long-time returning issue. Do you have some test cases that compares the results of the code against NOAA solar calculator? I can also write the tests but I thought I should ask as you have probably already checked it during the process. That are some PEP8 issues which I will comment them inside the code. Some of them are repetitive and all of them are easy to fix. |
There was a problem hiding this comment.
@devngc I added a couple of comments to your code. I'm not sure which editor are you using but most of them has their own plugins to check for style issues and there are plugins like autopep8 which will fix most of them for you. Thanks again for the PR. Cheers.
ladybug/sunpath.py
Outdated
|
|
||
| if hourAngle == 0.0 or hourAngle == -180.0 or hourAngle == 180.0: | ||
| # Degrees | ||
| zenith = math.degrees(math.acos(math.sin(self._latitude)*math.sin(math.radians(solDec))+math.cos(self._latitude)*math.cos(math.radians(solDec))*math.cos(math.radians(hourAngle)))) |
There was a problem hiding this comment.
- Add a white space before and after * - / +
- Break down the line into smaller segments. We us 89 characters.
ladybug/sunpath.py
Outdated
|
|
||
| # Degrees | ||
| if hourAngle>0: |
There was a problem hiding this comment.
white space before and after >
ladybug/sunpath.py
Outdated
|
|
||
| # Degrees | ||
| if hourAngle>0: | ||
| azimuth = math.degrees(math.acos(((math.sin(self._latitude)*math.cos(math.raradians(zenith)))-math.sin(math.radians(solDec)))/(math.cos(self._l_latitude)*math.sin(math.radians(zenith)))))+180 % 360 |
There was a problem hiding this comment.
The same here for white-space and the length of the line.
ladybug/sunpath.py
Outdated
|
|
||
| a = 1 if (month < 3) else 0 | ||
| y = year + 4800 - a | ||
| m = month + 12 * a - 3 | ||
|
|
||
| julianDay = day + math.floor((153 * m + 2) / 5) + 59 | ||
| def findFractionOf24(hour,minute): |
There was a problem hiding this comment.
White space is missing after ,
ladybug/sunpath.py
Outdated
| """ | ||
| This function calculates the fraction of the 24 hour the provided time represents | ||
| 1440 is total the number of minutes in a 24 hour cycle. | ||
| args |
There was a problem hiding this comment.
: is missing after args
ladybug/sunpath.py
Outdated
| """ | ||
|
|
||
| # Making a list of years from the year 1900 | ||
| years = [item for item in range(1900, year)] |
There was a problem hiding this comment.
What about years = range(1900, year)
ladybug/sunpath.py
Outdated
| # Making a list of years from the year 1900 | ||
| years = [item for item in range(1900, year)] | ||
|
|
||
| def is_leap_year(year): |
There was a problem hiding this comment.
The name of the method should be def isLeapYear
|
Hi @mostaphaRoudsari, Did you give it a try? Does it work at your end? and Does it produce more accurate altitude and azimuth? |
|
@mostaphaRoudsari, |
|
Didn't check the GH file. Will do later today after my class. If you're using Atom then here are the packages that I use which includes autopep8:
|
|
@mostaphaRoudsari, |
ladybug/sunpath.py
Outdated
|
|
||
| if hourAngle == 0.0 or hourAngle == -180.0 or hourAngle == 180.0: | ||
| # Degrees | ||
| zenith = math.degrees(math.acos \ |
There was a problem hiding this comment.
This is not major but you don't need \ inside parentheses.
ladybug/sunpath.py
Outdated
| @@ -683,3 +756,21 @@ def __repr__(self): | |||
| self.sunVector.y, | |||
| self.sunVector.z | |||
| ) | |||
|
|
|||
|
|
|||
| # Following are test cases to compare altitude and azimuth values with NOAA calculator | |||
There was a problem hiding this comment.
Thanks for adding these cases. They either need to be written as a proper test cases or at least we should make sure that they won't be execute on import. To achieve that you can use if __name__ == '__main__:. If you don't know the concept watch this video.
Once you add the if statement I can merge this. I will change them to proper test cases afterwards and you can follow that as an example for later.
Thanks again for all the great work @devngc!
I managed to run those packages eventuallly. So this file is checked for PEP8 for issues. Now, speaking of test cases, the main() is not there in the file. Do you want me to create that function and put all the code inside it. I throught I may invite changes in other files that depend on this one so I thought I would ask first. Also, this will again raise line length related PEP8 issues. For the time being, I commented out the test cases. Pleaase let me know if I should put everything inside a main(). Also, In order to run this file to check everythng, I need to write import __init__ as ladybug or else it throws import errror. Just sharing. This is may be a gap in my understanding about the way core and [+] libraries are paired.
|
I managed to run those packages eventually. So this file is checked for PEP8 for issues. Now, speaking of test cases, the main() is not there in the file. Do you want me to create that function and put all the code inside it? I thought it may invite changes in other files that depend on this one so I thought I would ask first. Also, this will again raise line length related PEP8 issues. For the time being, I commented out the test cases. Please let me know if I should put everything in a main(). Also, In order to run this file to check everything, I need to write |
|
|
|
@devngc it all looks good. I'm going to merge this now and move your test cases to test folder later. Once we have that we can set-up call and discuss some of the details here and there. Great job overall! |
|
@mostaphaRoudsari, |
These changes have mainly been directed towards improving the altitude and azimuth calculation. @mostaphaRoudsari This is my first PR for a [+] version. Kindly let me know if I am supposed to provide any other file.