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.

Sign up for GitHub

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

Simplify and future-proof numba import in spa.py #1944

Merged
merged 8 commits into from
Mar 13, 2024
Merged

Simplify and future-proof numba import in spa.py #1944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4943d17
remove numba version checking code
kandersolar Jan 4, 2024
a10ebd1
set minimum numba in setup.py requirements
kandersolar Jan 4, 2024
1aaf2da
remove same version check code in test_spa.py
kandersolar Jan 4, 2024
54d2b86
linter
kandersolar Jan 4, 2024
df450c7
fix numba version check in `conftest.py`
kandersolar Feb 23, 2024
dfd383b
add `noqa: F401` to other import checks
kandersolar Feb 23, 2024
5dac262
Merge remote-tracking branch 'upstream/main' into numba-simplify
kandersolar Mar 5, 2024
ef68240
set minimum numba in pyproject.toml
kandersolar Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions pvlib/spa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,15 @@ def nocompile(*args, **kwargs):

if os.getenv('PVLIB_USE_NUMBA', '0') != '0':
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if os.getenv('PVLIB_USE_NUMBA', '0') != '0':
if os.getenv('PVLIB_USE_NUMBA') is not None:

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this change would require some corresponding changes in the part of solarposition.py that controls how spa.py is imported (by setting this environment variable to '0' or '1'), so I'm inclined to leave this as-is for now. It will be addressed someday with #1060.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize that it could be defined as zero (I just assumed it would be undefined). Fine with me to just leave it.

try:
from numba import jit, __version__
from numba import jit
except ImportError:
warnings.warn('Could not import numba, falling back to numpy ' +
'calculation')
jcompile = nocompile
USE_NUMBA = False
else:
major, minor = __version__.split('.')[:2]
if int(major + minor) >= 17:
# need at least numba >= 0.17.0
jcompile = jit
USE_NUMBA = True
else:
warnings.warn('Numba version must be >= 0.17.0, falling back to ' +
'numpy')
jcompile = nocompile
USE_NUMBA = False
jcompile = jit
USE_NUMBA = True
else:
jcompile = nocompile
USE_NUMBA = False
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for inconsistent usage of upper vs. lower case, e.g., USE_NUMBA vs. has_numba?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I see is is that USE_NUMBA is a module-level variable used inside a function far away from where it is defined while has_numba is used once in the same scope immediately after its definition. But of course it's just style :)

Expand Down
22 changes: 8 additions & 14 deletions pvlib/tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@


try:
import ephem
import ephem # noqa: F401
has_ephem = True
except ImportError:
has_ephem = False
Expand All @@ -129,7 +129,7 @@

def has_spa_c():
try:
from pvlib.spa_c_files.spa_py import spa_calc
from pvlib.spa_c_files.spa_py import spa_calc # noqa: F401
except ImportError:
return False
else:
Expand All @@ -139,20 +139,14 @@
requires_spa_c = pytest.mark.skipif(not has_spa_c(), reason="requires spa_c")


def has_numba():
try:
import numba
except ImportError:
return False
else:
vers = numba.__version__.split('.')
if int(vers[0] + vers[1]) < 17:
return False
else:
return True
try:
import numba # noqa: F401
has_numba = True
except ImportError:
has_numba = False

Check warning on line 146 in pvlib/tests/conftest.py

View check run for this annotation

Codecov / codecov/patch

pvlib/tests/conftest.py#L145-L146 

Added lines #L145 - L146 were not covered by tests


requires_numba = pytest.mark.skipif(not has_numba(), reason="requires numba")
requires_numba = pytest.mark.skipif(not has_numba, reason="requires numba")


try:
Expand Down
20 changes: 5 additions & 15 deletions pvlib/tests/test_spa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,7 @@
import pandas as pd

import unittest
import pytest


try:
from numba import __version__ as numba_version
numba_version_int = int(numba_version.split('.')[0] +
numba_version.split('.')[1])
except ImportError:
numba_version_int = 0
from .conftest import requires_numba


times = (pd.date_range('2003-10-17 12:30:30', periods=1, freq='D')
Expand Down Expand Up @@ -390,17 +382,15 @@ def test_julian_day(self):
assert_almost_equal(JD, self.spa.julian_day(unixtimes)[0], 6)


@pytest.mark.skipif(numba_version_int < 17,
reason='Numba not installed or version not >= 0.17.0')
@requires_numba
class NumbaSpaTest(unittest.TestCase, SpaBase):
"""Import spa, compiling to numba, and run tests"""
@classmethod
def setUpClass(self):
os.environ['PVLIB_USE_NUMBA'] = '1'
if numba_version_int >= 17:
import pvlib.spa as spa
spa = reload(spa)
self.spa = spa
import pvlib.spa as spa
spa = reload(spa)
self.spa = spa

@classmethod
def tearDownClass(self):
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ optional = [
'cython',
'ephem',
'nrel-pysam',
'numba',
'numba >= 0.17.0',
'solarfactors',
'statsmodels',
]
Expand Down
Empty file modified setup.py
100755 → 100644
View file
Open in desktop
Empty file.
Loading