-
Notifications
You must be signed in to change notification settings - Fork 1.1k
more asv tests for solar position, fix fuentes asv bug #1059
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
Changes from 11 commits
9a8a70c
3e8e256
1497f11
2e9e8e2
9e66ae7
8fe9d42
e516097
b3cab54
90387b4
f03796b
519fb6b
a336d2a
48143e4
5c05818
f141b58
298ed03
7f99c06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| ASV benchmarks for solarposition.py | ||
| """ | ||
|
|
||
| import datetime | ||
| import pandas as pd | ||
| import pvlib | ||
| from pvlib import solarposition | ||
|
|
@@ -19,10 +20,12 @@ class SolarPosition: | |
|
|
||
| def setup(self): | ||
| self.times = pd.date_range(start='20180601', freq='1min', | ||
| periods=14400) | ||
| periods=14400) # 100 days | ||
| self.times_localized = self.times.tz_localize('Etc/GMT+7') | ||
| self.lat = 35.1 | ||
| self.lon = -106.6 | ||
| self.times_daily = pd.date_range( | ||
| start='20180601', freq='24h', periods=100, tz='Etc/GMT+7') | ||
|
|
||
| # GH 512 | ||
| def time_ephemeris(self): | ||
|
|
@@ -33,11 +36,51 @@ def time_ephemeris_localized(self): | |
| solarposition.ephemeris(self.times_localized, self.lat, self.lon) | ||
|
|
||
| def time_spa_python(self): | ||
| solarposition.spa_python(self.times_localized[::5], self.lat, self.lon) | ||
| solarposition.spa_python(self.times_localized, self.lat, self.lon) | ||
|
|
||
| def time_pyephem(self): | ||
| solarposition.pyephem(self.times_localized, self.lat, self.lon) | ||
|
|
||
| def time_sun_rise_set_transit_spa(self): | ||
| sun_rise_set_transit_spa(self.times_localized[::30], | ||
| self.lat, self.lon) | ||
| sun_rise_set_transit_spa(self.times_daily, self.lat, self.lon) | ||
|
|
||
| def time_sun_rise_set_transit_ephem(self): | ||
| solarposition.sun_rise_set_transit_ephem( | ||
| self.times_daily, self.lat, self.lon) | ||
|
|
||
| def time_sun_rise_set_transit_geometric(self): | ||
| dayofyear = self.times_daily.dayofyear | ||
| declination = solarposition.declination_spencer71(dayofyear) | ||
| equation_of_time = solarposition.equation_of_time_spencer71(dayofyear) | ||
| solarposition.sun_rise_set_transit_geometric( | ||
| self.times_daily, self.lat, self.lon, declination, | ||
| equation_of_time) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what to think about running more than one function in a single benchmark. It would be a little awkward/repetitive to run benchmark each step of a sequence individually, but it seems like the rule of thumb "only test one thing at a time" for unit tests ought to apply to benchmarks as well. That said, any benchmark is better than no benchmark!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also consider calculating
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general I agree. In this case I wanted a more direct comparison to the other |
||
|
|
||
| def time_nrel_earthsun_distance(self): | ||
| solarposition.nrel_earthsun_distance(self.times_localized) | ||
|
|
||
| def time_calc_time(self): | ||
| # test calc_time for finding times at which sun is 3 degrees | ||
| # above the horizon. | ||
| three_degrees = 0.05235987755982988 | ||
| # Tucson sunrise at 6:08 AM MST, 13:08 UTC according to google. | ||
| solarposition.calc_time( | ||
| datetime.datetime(2020, 9, 14, 12), | ||
| datetime.datetime(2020, 9, 14, 15), | ||
|
||
| 32.2, | ||
| -110.9, | ||
| 'alt', | ||
| three_degrees, | ||
| ) | ||
| # datetime.datetime(2020, 9, 14, 13, 24, 13, 861913, tzinfo=<UTC>) | ||
|
|
||
| # Tucson sunset at 6:30 PM MST, 01:30 UTC according to google | ||
| pvlib.solarposition.calc_time( | ||
|
||
| datetime.datetime(2020, 9, 14, 22), | ||
| datetime.datetime(2020, 9, 15, 4), | ||
| 32.2, | ||
| -110.9, | ||
| 'alt', | ||
| three_degrees, | ||
| ) | ||
| # datetime.datetime(2020, 9, 15, 1, 13, 2, 720384, tzinfo=<UTC>) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| """ASV benchmarks for solarposition.py using numba. | ||
| We use a separate module so that we can control the pvlib import process | ||
| using an environment variable. This will force pvlib to compile the numba | ||
| code during setup. | ||
| Try to keep relevant sections in sync with benchmarks/solarposition.py | ||
| """ | ||
|
|
||
| from pkg_resources import parse_version | ||
| import pandas as pd | ||
|
|
||
| import os | ||
| os.environ['PVLIB_USE_NUMBA'] = '1' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the environment var might not be needed now that you added numba to the env specs
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With setting the environment variable I see this: Without setting the environment variable (comment out the line), I see this: |
||
|
|
||
|
|
||
| import pvlib # NOQA: E402 | ||
| from pvlib import solarposition # NOQA: E402 | ||
|
|
||
|
|
||
| if parse_version(pvlib.__version__) >= parse_version('0.6.1'): | ||
| sun_rise_set_transit_spa = solarposition.sun_rise_set_transit_spa | ||
| else: | ||
| sun_rise_set_transit_spa = solarposition.get_sun_rise_set_transit | ||
|
|
||
|
|
||
| class SolarPositionNumba: | ||
|
|
||
| def setup(self): | ||
| self.times = pd.date_range(start='20180601', freq='1min', | ||
| periods=14400) # 100 days | ||
| self.times_localized = self.times.tz_localize('Etc/GMT+7') | ||
| self.lat = 35.1 | ||
| self.lon = -106.6 | ||
| self.times_daily = pd.date_range( | ||
| start='20180601', freq='24h', periods=100, tz='Etc/GMT+7') | ||
|
|
||
| def time_spa_python(self): | ||
| solarposition.spa_python( | ||
| self.times_localized, self.lat, self.lon, how='numba') | ||
|
|
||
| def time_sun_rise_set_transit_spa(self): | ||
| sun_rise_set_transit_spa( | ||
| self.times_daily, self.lat, self.lon, how='numba') | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep all the packages in the "minimum" list pinned to something, even if we don't actually have a minimum? That way any changes to numba that affect our performance will show up in one env but not the other.
Side note: I see that spa.py mentions a minimum numba version that isn't reflected in setup.py.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That minimum version is still accurate, as far as I know, but I'd be surprised if it's compatible with the rest of our minimum requirements.
conda search -c main numba | grep py36shows that numba 0.36.1 is the oldest package compatible with python 3.6 and numpy 1.12. The numba git history says 0.36.1 was released on Dec 7, 2017. (0.17.0 was released Feb 3, 2015.) I'll add that to the asv conf.I'd be fine with also adding a minimum numba requirement to setup.py, but I think that should be done in combination with changes to the import logic in spa.py. In particular, I don't like the numpy fallback - that led to hard to track down errors when developing this. It would also be worth reviewing modern numba best practices. If we're adding a minimum numba requirement to setup.py then we should probably be testing against it too. More than I want to tackle in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1060