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

mikofski · 2018-09-12T20:46:00Z

closes spa_c_files don't build with VC14 (Python 3.5) #168
get source and header in setup.py
replace "timezone" with "timezone" to fix the bug
also update readme
also remove a lot of extra whitespace and tabs

Signed-off-by: Mark Mikofski [email protected]

* closes pvlib#168 * get source and header in setup.py * replace "timezone" with "timezone" to fix the bug * also update readme * also remove a lot of extra whitespace and tabs Signed-off-by: Mark Mikofski <[email protected]>

wholmgren · 2018-09-12T21:12:14Z

pvlib/spa_c_files/README.md

+** Due to licensing issues, the [SPA C files](http://www.nrel.gov/midc/spa) can
+_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. **


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

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

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?

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.

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.

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.

* add enumeration for function type, it was reading garbage, so you never new exacton which calculations SPA was doing, and you would get garbage values for incidence, sunrise, transit, and sunset times * set function SPA_ALL, and pass extra args in as optional keyword args for slope, delta_ut1, azm_rotation, and atmos_refract * add example file that user can run to test build

wholmgren · 2018-09-12T21:55:38Z

pvlib/spa_c_files/README.md

-
-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


http://www.nrel.gov/midc/spa forwards to https://midcdmz.nrel.gov/

looks like https://midcdmz.nrel.gov/spa/ is now the correct url

…e email, (c)

mikofski · 2018-09-12T22:18:56Z

okay, the user is forced to see the license and push a key to install now, here's what they see:

>python setup.py build_ext --inplace
# NOTICE

Copyright (c) 2008-2011 Alliance for Sustainable Energy, LLC, All Rights Reserved

The Solar Position Algorithm ("Software") is code in development prepared by
employees of the Alliance for Sustainable Energy, LLC, (hereinafter the
"Contractor"), under Contract No. DE-AC36-08GO28308 ("Contract") with the
U.S. Department of Energy (the "DOE"). The United States Government has been
granted for itself and others acting on its behalf a paid-up, non-exclusive,
irrevocable, worldwide license in the Software to reproduce, prepare
derivative works, and perform publicly and display publicly. Beginning five
(5) years after the date permission to assert copyright is obtained from the
DOE, and subject to any subsequent five (5) year renewals, the United States
Government is granted for itself and others acting on its behalf a paid-up,
non-exclusive, irrevocable, worldwide license in the Software to reproduce,
prepare derivative works, distribute copies to the public, perform publicly
and display publicly, and to permit others to do so. If the Contractor ceases
to make this computer software available, it may be obtained from DOE's
Office of Scientific and Technical Information's Energy Science and
Technology Software Center (ESTSC) at P.O. Box 1020, Oak Ridge, TN
37831-1020. THIS SOFTWARE IS PROVIDED BY THE CONTRACTOR "AS IS" AND ANY
EXPRESS OR IMPLIED WARRANTIES, INCLUDING BUT NOT LIMITED TO, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL THE CONTRACTOR OR THE U.S. GOVERNMENT BE LIABLE
FOR ANY SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER,
INCLUDING BUT NOT LIMITED TO CLAIMS ASSOCIATED WITH THE LOSS OF DATA OR
PROFITS, WHICH MAY RESULT FROM AN ACTION IN CONTRACT, NEGLIGENCE OR OTHER
TORTIOUS CLAIM THAT ARISES OUT OF OR IN CONNECTION WITH THE ACCESS, USE OR
PERFORMANCE OF THIS SOFTWARE.

The Software is being provided for internal, noncommercial purposes only and
shall not be re-distributed. Please contact [Jean
Schulte](mailto:[email protected]) in the NREL Commercialization and
Technology Transfer Office for information concerning a commercial license to
use the Software.

As a condition of using the Software in an application, the developer of the
application agrees to reference the use of the Software and make this Notice
readily accessible to any end-user in a Help|About screen or equivalent manner.

Enter your name to accept the NREL LICENSE: <the user has to enter something here>

Then they have to press a key to continue.

ok?

cwhanse · 2018-09-12T22:45:40Z

I'm not comfortable with the auto-download, I would be more comfortable with if we sought and received NREL's agreement that the auto-download satisfies their request for registration. I don't think we should merge until we have their approval. I suspect they will not be pleased with the pvlib-python user name and will want individuals and organizations.

I'd be happy to contact NREL about this.

I appreciate and agree with the motivation to make the build convenient for users, especially with the timezone vs. time_zone behavior.

mikofski · 2018-09-12T22:46:58Z

@cwhanse sounds good , FYI: I changed it so the user has to enter their name now

wholmgren · 2018-09-12T22:51:52Z

Thanks @mikofski. Can we do something similar for all of the fields?

print('Enter the following to accept the NREL LICENSE:')
VALUES['name'] = input('Name: ')
VALUES['country'] = input('Country: ')
VALUES['company'] = input('Company: ')
VALUES['software'] = input('Software (enter pvlib-python if not bundling with another application): ')

mikofski · 2018-09-12T22:52:24Z

@wholmgren I responded to some of your comments that are hidden as outdata above I guess you saw them?

wholmgren · 2018-09-12T22:55:16Z

@mikofski confused about your comment above about the CI building it -- I don't think it is.

In any case, it didn't work for me:

$ cd pvlib/spa_c_files
$ python setup.py build_ext --inplace
$ cd ..
$ cd ..
$ pip install -e .
$ pytest pvlib/test/test_solarposition.py
============================================================================================================ test session starts =============================================================================================================
platform darwin -- Python 3.6.5, pytest-3.6.3, py-1.5.4, pluggy-0.6.0
rootdir: /Users/holmgren/git_repos/pvlib-python, inifile:
plugins: xdist-1.22.2, mock-1.10.0, forked-0.2, cov-2.6.0
collected 30 items

pvlib/test/test_solarposition.py FF...................x........                                                                                                                                                                        [100%]

================================================================================================================== FAILURES ==================================================================================================================
____________________________________________________________________________________________________________ test_spa_c_physical _____________________________________________________________________________________________________________

expected_solpos =                       elevation  apparent_zenith     azimuth  apparent_elevation
2003-10-17T12:30:30Z  39.872046        50.111622  194.340241           39.888378

    @requires_spa_c
    def test_spa_c_physical(expected_solpos):
        times = pd.date_range(datetime.datetime(2003,10,17,12,30,30),
                              periods=1, freq='D', tz=golden_mst.tz)
        ephem_data = solarposition.spa_c(times, golden_mst.latitude,
                                         golden_mst.longitude,
                                         pressure=82000,
>                                        temperature=11)

pvlib/test/test_solarposition.py:59:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
pvlib/solarposition.py:203: in spa_c
    delta_t=delta_t
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   ???
E   TypeError: spa_calc() takes at least 13 positional arguments (6 given)

spa_py.pyx:3: TypeError
__________________________________________________________________________________________________________ test_spa_c_physical_dst ___________________________________________________________________________________________________________

expected_solpos =                       elevation  apparent_zenith     azimuth  apparent_elevation
2003-10-17T12:30:30Z  39.872046        50.111622  194.340241           39.888378

    @requires_spa_c
    def test_spa_c_physical_dst(expected_solpos):
        times = pd.date_range(datetime.datetime(2003,10,17,13,30,30),
                              periods=1, freq='D', tz=golden.tz)
        ephem_data = solarposition.spa_c(times, golden.latitude,
                                         golden.longitude,
                                         pressure=82000,
>                                        temperature=11)

pvlib/test/test_solarposition.py:71:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
pvlib/solarposition.py:203: in spa_c
    delta_t=delta_t
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   ???
E   TypeError: spa_calc() takes at least 13 positional arguments (6 given)

spa_py.pyx:3: TypeError

mikofski · 2018-09-12T23:28:35Z

confused about your comment above about the CI building it -- I don't think it is.

no I didn't mean it's building the spa-c files, I meant it is building pvlib-python without the spa-c files, which I think is what we would expect, right? and it is consistently building pvlib-python everytime AFAICT

In any case, it didn't work for me:

weird, same for me on linux, but seems to work on windows? is this a red herring? grrrrr

wholmgren · 2018-09-12T23:31:13Z

@cwhanse @mikofski I support asking NREL if the latest interface meets their requirements. Here's what it looked like after I pulled @mikofski's latest changes that require the user to enter all information:

$ pwd
/Users/holmgren/git_repos/pvlib-python/pvlib/spa_c_files
$ python setup.py build_ext --inplace
# NOTICE

Copyright (c) 2008-2011 Alliance for Sustainable Energy, LLC, All Rights Reserved

The Solar Position Algorithm ("Software") is code in development prepared by
employees of the Alliance for Sustainable Energy, LLC, (hereinafter the
"Contractor"), under Contract No. DE-AC36-08GO28308 ("Contract") with the
U.S. Department of Energy (the "DOE"). The United States Government has been
granted for itself and others acting on its behalf a paid-up, non-exclusive,
irrevocable, worldwide license in the Software to reproduce, prepare
derivative works, and perform publicly and display publicly. Beginning five
(5) years after the date permission to assert copyright is obtained from the
DOE, and subject to any subsequent five (5) year renewals, the United States
Government is granted for itself and others acting on its behalf a paid-up,
non-exclusive, irrevocable, worldwide license in the Software to reproduce,
prepare derivative works, distribute copies to the public, perform publicly
and display publicly, and to permit others to do so. If the Contractor ceases
to make this computer software available, it may be obtained from DOE's
Office of Scientific and Technical Information's Energy Science and
Technology Software Center (ESTSC) at P.O. Box 1020, Oak Ridge, TN
37831-1020. THIS SOFTWARE IS PROVIDED BY THE CONTRACTOR "AS IS" AND ANY
EXPRESS OR IMPLIED WARRANTIES, INCLUDING BUT NOT LIMITED TO, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL THE CONTRACTOR OR THE U.S. GOVERNMENT BE LIABLE
FOR ANY SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER,
INCLUDING BUT NOT LIMITED TO CLAIMS ASSOCIATED WITH THE LOSS OF DATA OR
PROFITS, WHICH MAY RESULT FROM AN ACTION IN CONTRACT, NEGLIGENCE OR OTHER
TORTIOUS CLAIM THAT ARISES OUT OF OR IN CONNECTION WITH THE ACCESS, USE OR
PERFORMANCE OF THIS SOFTWARE.

The Software is being provided for internal, noncommercial purposes only and
shall not be re-distributed. Please contact [Jean
Schulte](mailto:[email protected]) in the NREL Commercialization and
Technology Transfer Office for information concerning a commercial license to
use the Software.

As a condition of using the Software in an application, the developer of the
application agrees to reference the use of the Software and make this Notice
readily accessible to any end-user in a Help|About screen or equivalent manner.


Enter the following information to accept the NREL LICENSE ...

Name: Will Holmgren
Company (or enter "Individual"): University of Arizona
Country: USA
Email (optional): [email protected]
running build_ext
building 'spa_py' extension
gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -I/Users/holmgren/miniconda3/envs/pvlibfx36/include -arch x86_64 -I/Users/holmgren/miniconda3/envs/pvlibfx36/include -arch x86_64 -I/Users/holmgren/git_repos/pvlib-python/pvlib/spa_c_files -I/Users/holmgren/miniconda3/envs/pvlibfx36/include/python3.6m -c spa_py.c -o build/temp.macosx-10.9-x86_64-3.6/spa_py.o
gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -I/Users/holmgren/miniconda3/envs/pvlibfx36/include -arch x86_64 -I/Users/holmgren/miniconda3/envs/pvlibfx36/include -arch x86_64 -I/Users/holmgren/git_repos/pvlib-python/pvlib/spa_c_files -I/Users/holmgren/miniconda3/envs/pvlibfx36/include/python3.6m -c spa.c -o build/temp.macosx-10.9-x86_64-3.6/spa.o
gcc -bundle -undefined dynamic_lookup -Wl,-rpath,/Users/holmgren/miniconda3/envs/pvlibfx36/lib -L/Users/holmgren/miniconda3/envs/pvlibfx36/lib -headerpad_max_install_names -Wl,-rpath,/Users/holmgren/miniconda3/envs/pvlibfx36/lib -L/Users/holmgren/miniconda3/envs/pvlibfx36/lib -headerpad_max_install_names -arch x86_64 build/temp.macosx-10.9-x86_64-3.6/spa_py.o build/temp.macosx-10.9-x86_64-3.6/spa.o -L/Users/holmgren/miniconda3/envs/pvlibfx36/lib -o /Users/holmgren/git_repos/pvlib-python/pvlib/spa_c_files/spa_py.cpython-36m-darwin.so

wholmgren · 2018-09-12T23:33:56Z

no I didn't mean it's building the spa-c files, I meant it is building pvlib-python without the spa-c files, which I think is what we would expect, right? and it is consistently building pvlib-python everytime AFAICT

Great. Let's not add this to the CI.

No clue about the platform/test issues. Sorry.

mikofski · 2018-09-12T23:37:03Z

No clue about the platform/test issues. Sorry.

It's "time_zone" now, not "timezone"!

pdb to the rescue!

wholmgren · 2018-09-12T23:45:07Z

pvlib/spa_c_files/setup.py

+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')


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

mikofski · 2018-09-12T23:58:05Z

OK, works now in linux too, thanks for catching that! I reverted the Python API in pvlib.solarposition.spa_c to continue to use timezone without the underscore, so it's only the cython version in pvlib.spa_c_files.spa_py.spa_calc that confusingly takes a timezone but outputs a time_zone, sorry! And I changed my mind again, since solarposition.spa_c is the only caller for spa_c_files.spa_calc.

~~I changed the input to pvlib.spa_c_files.spa_py.spa_calc to be timezone~~
I renamed the time_zone column in the raw output from pvlib.solarposition.spa_c to be timezone
I updated the call from pvlib.solarposition.spa_c to pvlib.spa_c_files.spa_py.spa_calc to use time_zone

I thought this would be the minimum API change, assuming that no one ever calls pvlib.spa_c_files.spa_py.spa_calc directly, because if they do, and they actually use the timezone output then it will now have an underscore. They'll be alerted to this when they try to call it without the underscore and get a confusing error.

~~Alternately,~~ to be slightly less confusing, I ~~could~~ have kept the input to pvlib.spa_c_files.spa_py.spa_calc to be time_zone, and changed how it's called from pvlib.solarposition.spa_c to use the time_zone keyword arg.

mikofski · 2018-09-13T00:04:33Z

See this explanation from the cython users google group - evidently , only a windows issue, and only since Python-3.5, pyconfig.h redefines timezone as a function.

…r args and returns

wholmgren · 2018-09-13T00:20:19Z

I confirm that the tests pass on my mac on commit 162e712

Probably want to add pvlib/spa_c_files/spa_py.c to .gitignore if my earlier comments about distributing it are no longer valid.

I think the proposed API is fine.

mikofski · 2018-09-13T00:28:15Z

I should probably leave, so I slightly revert the changes, and added spa_py.c back in after accidently deleting it, you might have you check your tests again, but make sure to purge your build folders, I had to on linux, it passes for me. I just thought it was better to have a consistent in/out api for spa_calc, even tho timezone isn't used that much.

wholmgren · 2018-09-13T00:35:14Z

Tests still pass. This seems sub-optimal but not a must fix:

$ python setup.py build_ext --inplace
$ git diff
diff --git a/pvlib/spa_c_files/spa_py.c b/pvlib/spa_c_files/spa_py.c
index 378c34c..b711b0d 100644
--- a/pvlib/spa_c_files/spa_py.c
+++ b/pvlib/spa_c_files/spa_py.c
@@ -4,10 +4,10 @@
 {
     "distutils": {
         "depends": [
-            "spa.h"
+            "/Users/holmgren/git_repos/pvlib-python/pvlib/spa_c_files/spa.h"
         ],
         "include_dirs": [
-            "."
+            "/Users/holmgren/git_repos/pvlib-python/pvlib/spa_c_files"
         ],
         "name": "spa_py",
         "sources": [

mikofski · 2018-09-13T00:38:14Z

I'm not a fan either, if rather drop the generated Cython, I can't think of a case where any user would use it

mikofski · 2018-09-13T01:27:32Z

I think that's only showing up on your machine after you build inplace and replace that spa_py.c file locally. It doesn't look that way on GitHub https://github.com/mikofski/pvlib-python/blob/fix_spa_c_files_timezone_compile_fail/pvlib/spa_c_files/spa_py.c

wholmgren · 2018-09-13T01:52:54Z

My objection is when I build the package it modifies a file that git tracks, so now I have a file that shows up under "not staged for commit" in my local repo. If I used this regularly it would be a problem for me because I use git commit -a all the time. But I don't, so I won't be a stickler about it. Maybe there is some difference between your platform and mine or maybe I'm not doing something right. In any case, if we're going to require cython to use spa_c then we could git rm spa_py.c as you suggested earlier and also add it to .gitignore and problem solved.

mikofski · 2018-09-13T02:26:29Z

Sorry, I misunderstood, yes the same annoying thing happens to me too. I vote to git remove it and then git ignore as you suggest.

wholmgren · 2018-09-13T02:31:11Z

What do you think of the LGTM alerts? No big deal for this?

* git rm spa_py.c to remove it from repo * add cython to option extra requires * use full paths in spa_c_files for sources * add line to bug fixes addressing pvlib#168

mikofski · 2018-09-13T05:29:26Z

What do you think of the LGTM alerts? No big deal for this?

I thought about this, it's only a concern for Python-2, b/c in Python-3, input is always coerced to string, but in Python-2, it calls evil eval. LGTM concern is straight from Python-2. But I think in this case it's okay, because:

the user probably should not try to build the spa-c files in Python-2. I don't know if the byte-strings will work without some compatibility fixes, and I don't have the patience for something that affects 1% of users
the user is using input on themselves, so if they happen to be using Python-2.7, and it magically works, and they happen to type rm -rf / as their name, then it's kind of their own fault

If we wanted we could just use python-future: from builtins import input but this still won't satisfy LGTM, probably we'd have to either drop Python-2 or do an ugly compat fix like this: from past.builtins import raw_input, and then replace input with raw_input everywhere.

or if we can't have six or future as requirements, then try something like this:

# see if raw_input is defined
try:
    raw_input():
except NameError:
    # this is Python-3
    raw_input = input

six also renames, raw_input to input to be compatible with Python-3, which I don't know if it will satisfy LGTM.

mikofski · 2018-09-13T06:25:07Z

Anyway, if you get approval from NREL, I think this ready. Any need for a lot of what's new updates? thanks!

cwhanse · 2018-09-13T13:40:40Z

@mikofski @wholmgren am trading emails with NREL will let you know if/when they approve.

wholmgren · 2018-09-13T15:24:26Z

@mikofski thanks for detailed explanation of the input checks. I agree we should leave it as input.

If you want to factor out the code that downloads the files from NREL then we can go ahead and merge. I don't view the iteration on the auto-download code as a waste of time because I think it's important that we can present to NREL a clear idea of how it would work. This PR is one part bug fix and another part new feature, so no problem splitting it up from that perspective.

Or we can give it some time (hours/days/weeks/months/years?) to see what they say. Up to you.

@cwhanse thanks for contacting NREL.

mikofski · 2018-09-13T15:53:58Z

OK, I agree, I'll factor out the download code.

And BTW, it works fine in Python-2

Also FYI: the timezone Python bug is already being tracked here: Python bugs issue: 24643

cwhanse · 2018-09-13T16:16:50Z

I agree with the factoring at this point.

mikofski · 2018-09-13T16:36:00Z

BTW: refactoring out the autodownload removes the input threat, which turns out it was real, b/c the setup.py byte-string patching does work on py2

Anyway, OK now?

mikofski · 2018-09-13T16:38:18Z

Here's the old code for a future PR, with the raw_input bug fixed.

wholmgren

Looks good. I confirmed it works as expected. I needed to already have the spa.c file manually downloaded from the website (no more auto-download). And the annoying git pollution is gone -- thanks Mark!

@cwhanse go ahead and merge if you approve.

pvlib/solarposition.py

cwhanse · 2018-09-14T18:48:14Z

pvlib/solarposition.py

@@ -206,7 +206,7 @@ def spa_c(time, latitude, longitude, pressure=101325, altitude=0,
    spa_df = pd.DataFrame(spa_out, index=time)

    if raw_spa_output:
-        return spa_df
+        return spa_df.rename(columns={'time_zone': 'timezone'})


Add a comment explaining why this is done

pvlib/spa_c_files/README.md

cwhanse · 2018-09-14T18:50:39Z

pvlib/spa_c_files/setup.py

+# patch spa.c
+with open(os.path.join(DIRNAME, 'spa.c'), 'rb') as f:
+    SPA_C = f.read()
+# replace timezone with time_zone to avoid a nameclash the function


with the function

cwhanse · 2018-09-14T18:51:55Z

Ready to go, except that I think a few inline comments would be wise to remind us why we change timezone

* explain that timezone is replaced with time_zone in spa_c docstring to avoid nameclash due to Python bug 24643 * explain why time_zone column renamed to timezone to match caller API

* that explains that we replace timezone with time_zone at build

stickler-ci · 2018-09-14T19:34:19Z

pvlib/solarposition.py

+    NREL SPA reference: http://rredc.nrel.gov/solar/codesandalgorithms/spa/
+    NREL SPA C files: https://midcdmz.nrel.gov/spa/
+
+    Note: The ``timezone`` field in the SPA C files is replaced with ``time_zone``


E501 line too long (82 > 79 characters)

stickler-ci · 2018-09-14T19:34:19Z

pvlib/solarposition.py

+
+    Note: The ``timezone`` field in the SPA C files is replaced with ``time_zone``
+    to avoid a nameclash with the function ``__timezone`` that is redefined by
+    Python>=3.5. This issue is `Python bug 24643 <https://bugs.python.org/issue24643>`_.


E501 line too long (88 > 79 characters)

wholmgren · 2018-09-14T20:14:52Z

Thanks @mikofski and @cwhanse. I think the additional comments will be helpful.

mikofski · 2018-09-16T02:36:09Z

@wholmgren AFAIK I think this is ready whenever, just waiting for approval from @cwhanse

cwhanse · 2018-09-16T15:48:48Z

Closes #168

BUG: ENH: update spa_files to patch timezone bug

3e6af01

* closes pvlib#168 * get source and header in setup.py * replace "timezone" with "timezone" to fix the bug * also update readme * also remove a lot of extra whitespace and tabs Signed-off-by: Mark Mikofski <[email protected]>

wholmgren reviewed Sep 12, 2018

View reviewed changes

mikofski added 2 commits September 12, 2018 14:35

ENH: mention test exampleg, fix azm_rotation typo

7ab8b64

wholmgren reviewed Sep 12, 2018

View reviewed changes

ENH: fix link, force reader to accept license, wrap long lines, updat…

1b4a73c

…e email, (c)

wholmgren added this to the 0.6.0 milestone Sep 12, 2018

wholmgren added the bug label Sep 12, 2018

ENH: ask user for their name, fail ungracefully

e2232f1

ENH: get all information to submit to DOE

feb1f36

wholmgren reviewed Sep 12, 2018

View reviewed changes

use timezone as arg, change output to timezone

162e712

add comment about __timezone macro, use consistent API in spa_calc fo…

d04dc51

…r args and returns

oops accidently deleted spa_py.c

78e4067

ENH: ignore cython generated spa_py.c file

05f4e45

* git rm spa_py.c to remove it from repo * add cython to option extra requires * use full paths in spa_c_files for sources * add line to bug fixes addressing pvlib#168

remove autodownload, revert readme

8298871

wholmgren approved these changes Sep 13, 2018

View reviewed changes

cwhanse requested changes Sep 14, 2018

View reviewed changes

mikofski added 4 commits September 14, 2018 12:06

with the function

44c2037

respond to comments

e33c8f3

* explain that timezone is replaced with time_zone in spa_c docstring to avoid nameclash due to Python bug 24643 * explain why time_zone column renamed to timezone to match caller API

Copy the comment from solarposition.py

83e5e5d

* that explains that we replace timezone with time_zone at build

Python>=3.5

3bde113

stickler-ci bot reviewed Sep 14, 2018

View reviewed changes

wrap long lines

fcf5ca4

cwhanse approved these changes Sep 16, 2018

View reviewed changes

cwhanse merged commit e3323b5 into pvlib:master Sep 16, 2018

mikofski deleted the fix_spa_c_files_timezone_compile_fail branch September 16, 2018 16:49

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

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

Conversation

mikofski commented Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikofski Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikofski commented Sep 12, 2018 • edited Loading

cwhanse commented Sep 12, 2018

mikofski commented Sep 12, 2018 • edited Loading

wholmgren commented Sep 12, 2018

mikofski commented Sep 12, 2018

wholmgren commented Sep 12, 2018

mikofski commented Sep 12, 2018

wholmgren commented Sep 12, 2018 • edited Loading

wholmgren commented Sep 12, 2018

mikofski commented Sep 12, 2018

Choose a reason for hiding this comment

mikofski commented Sep 12, 2018 • edited Loading

mikofski commented Sep 13, 2018

wholmgren commented Sep 13, 2018

mikofski commented Sep 13, 2018

wholmgren commented Sep 13, 2018 • edited Loading

mikofski commented Sep 13, 2018

mikofski commented Sep 13, 2018 • edited Loading

wholmgren commented Sep 13, 2018

mikofski commented Sep 13, 2018

wholmgren commented Sep 13, 2018

mikofski commented Sep 13, 2018 • edited Loading

mikofski commented Sep 13, 2018

cwhanse commented Sep 13, 2018

wholmgren commented Sep 13, 2018

mikofski commented Sep 13, 2018 • edited Loading

cwhanse commented Sep 13, 2018

mikofski commented Sep 13, 2018

mikofski commented Sep 13, 2018 • edited Loading

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhanse commented Sep 14, 2018 • edited Loading

stickler-ci bot Sep 14, 2018

Choose a reason for hiding this comment

stickler-ci bot Sep 14, 2018

Choose a reason for hiding this comment

wholmgren commented Sep 14, 2018 • edited Loading

mikofski commented Sep 16, 2018

cwhanse commented Sep 16, 2018

mikofski commented Sep 12, 2018 •

edited

Loading

mikofski Sep 12, 2018 •

edited

Loading

mikofski commented Sep 12, 2018 •

edited

Loading

mikofski commented Sep 12, 2018 •

edited

Loading

wholmgren commented Sep 12, 2018 •

edited

Loading

mikofski commented Sep 12, 2018 •

edited

Loading

wholmgren commented Sep 13, 2018 •

edited

Loading

mikofski commented Sep 13, 2018 •

edited

Loading

mikofski commented Sep 13, 2018 •

edited

Loading

mikofski commented Sep 13, 2018 •

edited

Loading

mikofski commented Sep 13, 2018 •

edited

Loading

cwhanse commented Sep 14, 2018 •

edited

Loading

wholmgren commented Sep 14, 2018 •

edited

Loading