-
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 all 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 |
---|---|---|
@@ -0,0 +1,46 @@ | ||
"""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' | ||
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 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: | ||
params = [1, 10, 100] # number of days | ||
param_names = ['ndays'] | ||
|
||
def setup(self, ndays): | ||
self.times = pd.date_range(start='20180601', freq='1min', | ||
periods=1440*ndays) | ||
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=ndays, tz='Etc/GMT+7') | ||
|
||
def time_spa_python(self, ndays): | ||
solarposition.spa_python( | ||
self.times_localized, self.lat, self.lon, how='numba') | ||
|
||
def time_sun_rise_set_transit_spa(self, ndays): | ||
sun_rise_set_transit_spa( | ||
self.times_daily, self.lat, self.lon, how='numba') |
Large diffs are not rendered by default.
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.
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!
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.
Also consider calculating
dayofyear
in the setup function because it's externalThere 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.
In general I agree. In this case I wanted a more direct comparison to the other
sun_rise_set_transit
functions, so I put all that in the test. Thedayofyear
calculation is part of a fair comparison. But I also see the argument to test each of them individually and add them up. For now, I renamed the existing function to make it clear it's for comparison.