Allowing spa_c() to use localized time index

[anomam]

Addresses #237 :
spa_c()'s docstring specifies that time can be localized, but the code only assumes 'UTC' time.

[anomam]

Hi @wholmgren , I just implemented a quick fix for an issue I raised a while back. Let me know what you think!
Also, I just noticed that continuous integration is not triggered in this PR when I push a commit, did I mess up something?

Thanks!

[mikofski]

I don't think the nrel_c option is tested in the build server though, or if it is, only python27, it only builds with msvc90 (vs2008), I wasn't able to build it for Python-3.5 using msvc140 (vs2015). I didn't try Python-3.3 or 3.4. see #168

[wholmgren]

@mikofski is correct -- the spa_c algorithm is not tested on travis or appveyor. You can run the tests on your own machine if the spa files are present when you build pvlib python. I don't want to support spa_c at all, but so long as we do, you should probably add a new test. Still, I am ok merging this in any state. Let me know when you want me to go ahead and click the button.

I'd still like to know why you Sunpower folks are using nrel_c instead of nrel_numpy or nrel_numba. nrel_c is slower than both on every machine I've tested them on.

[anomam]

@wholmgren thanks for your reply.
I guess I personally started using nrel_c because I was using the SPA website originally, and maybe I also had the assumption that the C code would be faster.
But it sounds like there's no reason to use it if nrel_numpy provides the exact same results even faster. Why do you guys keep it in the code considering all that?

[wholmgren]

Good question! I remember discussing this when we added the nrel_num* methods though I can't seem to find it right now. I think the consensus was that we should keep nrel_c since it was already working and some people might prefer nrel_c because the c code was written by the renewable energy gods on the hill in Golden. Maybe we should add a note to the function documentation with the relative speeds. I guess the benchmarks are somewhat buried at the bottom of the solar position jupyter notebook. I'd suggest that you rerun the benchmarks on your own platform, though.

In any case, it don't think it hurts to go ahead with this PR since you've already made the change. Up to you.

[adriesse]

I think it's great to have and keep multiple implementations that should and do give the same results. Great confidence booster! You could have a test that runs both and compares.

[wholmgren]

Good point on the confidence booster. The test suite does this in a if A=B and A=C then B=C kind of way. The problem is that it's difficult (impossible?) to run those tests via travis/appveyor since we can't redistribute NREL's spa files. I haven't manually run those tests in a long time.

[mikofski]

You can add nrel_c to travis.yml like this:

1. Add a script called get_spa.py to pvlib's top level. Here's an example, you can change the "name" and "company" fields to whatever you think is appropriate:

#! /usr/env/python

"""
get spa c source from NREL for travis
"""

import requests
import os
import logging

# logger
logging.basicConfig(level=logging.DEBUG)
LOGGER = logging.getLogger(__file__)
# constants
SPAC_URL = r'https://midcdmz.nrel.gov/apps/download.pl'  # spa.c source url
# register with NREL to download spa.c source
PAYLOAD = {
    'name': 'pvlib-python',  # <-- okay to set this
    'country': 'US',
    'company': 'pvlib,  # <-- okay to set this
    'software': 'SPA'
}
SPAH_URL = r'http://midcdmz.nrel.gov/spa/spa.h'  # spa.h source url
PVLIB_PATH = os.environ['PVLIB_PATH']  # path to PVLIB on travis
SPA_C_FILES = os.path.join(PVLIB_PATH, 'pvlib', 'spa_c_files')

if __name__ == "__main__":
    # get spa.c source
    LOGGER.debug('post payload to url: %s\n\tpayload:\n%s', SPAC_URL, PAYLOAD)
    r = requests.post(SPAC_URL, data=PAYLOAD)
    LOGGER.debug('post response: %r', r)
    # save spa.c source to PVLIB/SPA_C_FILES folder
    with open(os.path.join(SPA_C_FILES, 'spa.c'), 'wb') as f:
        f.write(r.content)
    LOGGER.debug('saved file: %r', f.name)
    # get spa.c source
    LOGGER.debug('get url: %s', SPAH_URL)
    r = requests.get(SPAH_URL)
    LOGGER.debug('get response: %r', r)
    # save spa.c source to PVLIB/SPA_C_FILES folder
    with open(os.path.join(SPA_C_FILES, 'spa.h'), 'wb') as f:
        f.write(r.content)
    LOGGER.debug('saved file: %r', f.name)
    LOGGER.debug('exiting %s', __file__)  # exit

2. Add a before_script section to your travis.yml to build the nrel_c binaries.

before_script:
  - pip install requests cython
  - export PVLIB_PATH=/home/travis/build/pvlib/pvlib-python
  - echo $PVLIB_PATH
  - python /home/travis/build/pvlib/pvlib-python/get_spa.py
  - BUILD_DIR=$PWD
  - echo leaving $BUILD_DIR
  - cd $PVLIB_PATH/pvlib/spa_c_files/
  - echo entered $PWD
  - python setup.py build_ext --inplace
  - cd $BUILD_DIR
  - echo entered $PWD

These scripts are adapted from UncertaintyWrapper, so they may be some redundancy or require some minor edits.

[wholmgren]

Interesting. Up to you all to make it happen if you want it. Not sure if we'd want a separate Travis build for this or not.

[mikofski]

the renewable energy gods on the hill

😆 🤣

[anomam]

Thanks for all the inputs everybody.
Since it seems like the 'nrel_num*' methods are already doing the job I've moved on to other urgent things but I'll try to come back to it when I have more time to work on the continuous integration update and the new tests to write (not trivial to me). Or feel free to take over this PR if you want!

Thanks,
Marc

[wholmgren]

I can merge this if you remove the update to the whatsnew doc. I can update the whatsnew for you when the next file gets into the repository.

I don't want to add the travis script at this time. Tracking down errors on the build matrix is already too time consuming for our current level of support.

[mikofski]

wondering if this is closed now, since I saw you branched it and made another commit (c881445)? Did you already merge it with #326 (2e2f7a8)?

[wholmgren]

No, I was going to make a PR to @anomam's branch, but got distracted. It can go into 0.4.5 if you resolve the conflict ASAP.

[mikofski]

I can't edit @anomam branch, but I think you can edit a pull request, it's like a GitHub option somewhere - https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/ - ah looks like Marc would have to allow it. Sorry 😦

[wholmgren]

closing in favor of #333.

[anomam]

Really sorry about this guys, I hope I didn't block anything and that you were able to do what you needed to do! @wholmgren

[wholmgren]

No worries. I should have merged it back when it was easy to do so. The github "squash-and-merge" process dropped your name from the commit for some dumb reason. Sorry.