Skip to content

BUG: ENH: update spa_files to patch timezone bug #576

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

Merged
merged 16 commits into from
Sep 16, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
36 changes: 20 additions & 16 deletions pvlib/spa_c_files/README.md
Original file line number Diff line number Diff line change
@@ -1,34 +1,38 @@
README
------

NREL provides a C implementation of the solar position algorithm
described in
[Reda, I.; Andreas, A. (2003). Solar Position Algorithm for Solar Radiation Applications. 55 pp.; NREL Report No. TP-560-34302](
http://www.nrel.gov/docs/fy08osti/34302.pdf).
NREL provides a C implementation of the solar position algorithm described in
[Reda, I.; Andreas, A. (2003). Solar Position Algorithm for Solar Radiation Applications. 55 pp.; NREL Report No. TP-560-34302](http://www.nrel.gov/docs/fy08osti/34302.pdf).

This folder contains the files required to make NREL's C code accessible
to the ``pvlib-python`` package. We use the Cython package to wrap NREL's SPA
implementation.

** Due to licensing issues, you must download the NREL C files from their
[website](http://www.nrel.gov/midc/spa) **

Download the ``spa.c`` and ``spa.h`` files from NREL,
and copy them into the ``pvlib/spa_c_files`` directory.
** Due to licensing issues, the [SPA C files](http://www.nrel.gov/midc/spa) can
Copy link
Member

Choose a reason for hiding this comment

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

_not_ be included in the pvlib-python distribution. The SPA C files will be
downloaded when you build the Python extension. By using this module you agree
to the NREL license in SPA_NOTICE. **
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned about adding this feature. Can we really say this and wash our hands of the license issue? I would be surprised. I'd believe we're in the clear if we pulled the license text, printed it, and waited for the user to input y.

Also not clear to me how this is supposed to work (or not work) with the spa related code in the root setup.py

Copy link
Member

Choose a reason for hiding this comment

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

(by this feature I mean the auto-download capability)

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'm concerned about adding this feature. Can we really say this and wash our hands of the license issue? I would be surprised.

but the LICENSED code is not being distributed with pvlib, so I don't see the concern?

I'd believe we're in the clear if we pulled the license text, printed it, and waited for the user to input y.

I guess we can do that

Also not clear to me how this is supposed to work (or not work) with the spa related code in the root setup.py

AFAIK there's no difference, everything is exactly the same way it was before. The user has to follow the instructions in the README. If they want to use the SPA C-files, then they need to somehow enter this folder and run

python setup.py build_ext --inplace

it's the only way to build it, and by doing this act, then implicitly agree to the license. If they don't want to use the SPA C-files they do nothing. AFAIK the rest of pvlib still works exactly the way it always did, what's different?

Copy link
Member

Choose a reason for hiding this comment

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

but the LICENSED code is not being distributed with pvlib, so I don't see the concern?

Maybe. But I don't want NREL to think "Holmgren, Mikofski, and the rest of those ^&*( pvlib-ers snaked their way around our license."

If we support automatic download, I'd also want a user to be required to input the form information rather than it being supplied as "pvlib-python" etc. NREL is presumably asking for this information from each user in order to justify funding to DOE. Putting a placeholder there doesn't seem right, regardless of whether we agree with NREL's strategy here or not.

AFAIK there's no difference, everything is exactly the same way it was before.

The root setup.py compiles the extension if the files were detected during a pip install . or python setup.py. Or at least it used to -- it's been years since I tried. Perhaps it's best to remove the capability of the root setup.py to do anything with this extension.

Copy link
Member Author

Choose a reason for hiding this comment

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

great idea, okay, it now asks for users name, and sends that, if no name, then it fails with some ugly message which contains the contents of spa.c which says something like:

Name must be atleast 2 valid characters.

Copy link
Member Author

@mikofski mikofski Sep 12, 2018

Choose a reason for hiding this comment

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

It looks like the root setup.py checks if the files are present so I think it will run okay.

It looks like the CI's had no problems building it either.

Also I just checked it on my PC, and it also works as expected:

  • if no spa.c or spa.h then you get this:

    WARNING: spa_c files not detected. See installation instructions for more information.

  • if yes spa.c or spa.h then it builds the extension.


There are a total of 5 files needed to compile the C code, described below:

* ``spa.c``: original C code from NREL
* ``spa.h``: header file for spa.c
* ``cspa_py.pxd``: a cython header file which essentially tells cython which parts of the main header file to pay attention to
* ``spa_py.pyx``: the cython code used to define both functions in the python namespace. NOTE: It is possible to provide user access to other paramters of the SPA algorithm through modifying this file
* ``cspa_py.pxd``: a cython header file which essentially tells cython which
parts of the main header file to pay attention to
* ``spa_py.pyx``: the cython code used to define both functions in the python
namespace. NOTE: It is possible to provide user access to other paramters of
the SPA algorithm through modifying this file
* ``setup.py``: a distutils file which performs the compiling of the cython code

The cython compilation process produces two files:
* ``spa_py.c``: an intermediate cython c file
* ``spa_py.so``: the python module which can be imported into a namespace
* ``spa_py.so`` or ``spa_py.<cpyver-platform>.pyd``: the python module which
can be imported into a namespace

To create the SPA Python extension, use the following shell command inside this
folder:

$ python setup.py build_ext --inplace

To process the original 5 files,
use the following shell command inside this folder

$ python setup.py build_ext --inplace
Executing the build command will also download the ``spa.c`` and ``spa.h``
files from NREL and copy them into the ``pvlib/spa_c_files`` directory.
58 changes: 29 additions & 29 deletions pvlib/spa_c_files/cspa_py.pxd
Original file line number Diff line number Diff line change
@@ -1,40 +1,40 @@
cdef extern from "spa.h":
ctypedef struct spa_data:
int year
int month
int day
int hour
int minute
double second
double delta_ut1
double delta_t
double timezone
double longitude
double latitude
ctypedef struct spa_data:
int year
int month
int day
int hour
int minute
double second
double delta_ut1
double delta_t
double time_zone
double longitude
double latitude

double elevation
double elevation

double pressure
double pressure

double temperature
double temperature

double slope
double slope

double azm_rotation
double azm_rotation

double atmos_refract
double atmos_refract

int function
int function

double e0
double e
double zenith
double azimuth_astro
double azimuth
double incidence
double e0
double e
double zenith
double azimuth_astro
double azimuth
double incidence

double suntransit
double sunrise
double sunset
double suntransit
double sunrise
double sunset

int spa_calculate(spa_data *spa)
int spa_calculate(spa_data *spa)
31 changes: 29 additions & 2 deletions pvlib/spa_c_files/setup.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,36 @@
#setup.py
# setup.py

import os
from urllib import request, parse
from distutils.core import setup
from distutils.extension import Extension
from Cython.Build import cythonize

DIRNAME = os.path.dirname(__file__)
SPA_C_URL = r'https://midcdmz.nrel.gov/apps/download.pl'
SPA_H_URL = r'https://midcdmz.nrel.gov/spa/spa.h'
VALUES = {
'name': 'pvlib-python',
'country': 'US',
'company': 'Individual',
'software': 'SPA'
}
DATA = parse.urlencode(VALUES).encode('ascii')

# get spa.c
REQ = request.Request(SPA_C_URL, DATA)
with request.urlopen(REQ) as response:
SPA_C = response.read()
SPA_C = SPA_C.replace(b'timezone', b'time_zone')
Copy link
Member

Choose a reason for hiding this comment

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

you might consider a comment here about why you're doing this and also reference the github issue.

with open(os.path.join(DIRNAME, 'spa.c'), 'wb') as f:
f.write(SPA_C)
# get spa.h
with request.urlopen(SPA_H_URL) as response:
SPA_H = response.read()
SPA_H = SPA_H.replace(b'timezone', b'time_zone')
with open(os.path.join(DIRNAME, 'spa.h'), 'wb') as f:
f.write(SPA_H)

setup(
ext_modules = cythonize([Extension("spa_py", ["spa_py.pyx",'spa.c'])])
ext_modules=cythonize([Extension('spa_py', ['spa_py.pyx', 'spa.c'])])
)
Loading