-
Notifications
You must be signed in to change notification settings - Fork 1.1k
spa license fixes #19
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
Conversation
I think this is ready, but it should definitely be tested in full by somebody else. |
pvlib/spa_c_files/spa.c | ||
pvlib/spa_c_files/spa.h | ||
pvlib/spa_c_files/spa_tester.c | ||
|
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 think pvlib/spa_c_files/spa_py.so
should be removed from the repo and also be added to the ignore file (also because the SPA tests are passing before SPA is installed!)
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.
Agreed, I'll fix this.
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.
Hm, on second thought I already removed the file and line 10 of .gitignore
is *.so
. You may be seeing the file because of some git oddity when switching between branches.
$ git ls-files pvlib/spa_c_files/
pvlib/spa_c_files/README.md
pvlib/spa_c_files/SPA_NOTICE.md
pvlib/spa_c_files/__init__.py
pvlib/spa_c_files/cspa_py.pxd
pvlib/spa_c_files/setup.py
pvlib/spa_c_files/spa_py.c
pvlib/spa_c_files/spa_py.pyx
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.
You are right, I see. Sorry that I was causing this confusion. The problem that the so
will stay in the file structure exists, because it is ignored and will stay at its place, if you switch branches.
I checked out your branch locally and tested it. Everything seems to be ok (the only difference to your install instructions was the use of However I'm not sure about a Windows install, which could be a problem in gereneral because of the compilation of the c code (but this is not a problem introduced by this pull request). |
I did testing on @wholmgren's branch. I think the confusion is coming from running the install from the local pvlib-python folder vs site-packages. When doing a pip install, the spa_c_files directory isn't created in site-packages when installing, so it isn't possible to go back and do a So, I think the only way to propagate this into the site-packages directory properly is using pip, and so I'd reccomend updating the README.md with this:
|
I updated the readme with @Calama-Consulting's suggestion and rebased on the numerous master changes. |
As discussed in #9, this PR:
spa.c
,spa.h
, andspatester.c
files.setup.py
so that it works with or without the spa files. I tested this, but see below.test_solarposition.py
to skip the spa tests if the files are missing. @bmu probably won't like the copy/paste test code, but I think that it can be fixed in0.2
.(Now resolved) Major issue: NREL made a few minor changes to the SPA code that will require modifying the cython code. From the most recent spa.c file:
This PR currently works with the 01-APR-2013 version of the spa code. It should probably be updated to work with the latest version. I'm guessing that it would be a hassle to make it work with both versions.This PR currently works with the 08-SEP-2014 version of the spa code. It does not work with the older code.