-
Notifications
You must be signed in to change notification settings - Fork 1.1k
NREL SPA license problems #9
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
Comments
My inclusion of it was based on it's original inclusion in the MATLAB version of the code, which carries the following liscence:
Which doesn't seem to maintain the license for the spa code. I will follow up on this with the appropriate folks at NREL, in the meantime, we should add a logger message indicating these limitations until it is cleared out. Does that make sense? Cheers |
Hm, sort of makes sense. Maybe I need more coffee. I don't see that the NREL C code is actually contained in PVLIB-Matlab, so I'm not sure how that license applies here. Since I wasn't very clear, I am specifically concerned about us including NREL's It seems as though the authors of the Another option would be to distribute the cython wrappers but make people download their own copies of the NREL code. Certainly not an ideal situation, but it could work for people that really want the speed improvements and are willing to do some manual installation. |
I'm working on a change that will allow the user to install spa themselves with a version of the .c and .h files that they download from the NREL website. The best way I can think of is to have the pypi version have the function disabled by default. If they want to install spa, you would need to download the full tarball from github, and run
where setup_spa is the originial setup file, after they copy the spa.c and spa.h into the appropriate directory. That is pretty messy, and there should be another way, I was thinking of something like this:
but setuptools doesn't copy all the required files into the spa_c_files in site-packages when it isn't explicitly asked to compile it on the first setup. Thoughts? |
On the first proposal: my guess is that if NREL won't let us put their files in the PyPI package, then they won't let us put them on the github repo either. If I'm wrong about that, then this idea could work. On the second proposal: I think that it would be easier in the end to actually go further and just split the spa wrappers into a separate package. Here's my thinking... Users would download a tarball or git clone the fairly simple spa repo, put their separately downloaded NREL files into that directory, and then run the spa repo's If the spa code is in a separate package, you only need to install once. No conflict with installing into an existing site-packages either. Also, while a determined user will be able to follow any reasonable spa installation instructions, the user will not be happy following those instructions for every pvlib update -- especially bug fix updates. Users of virtual environments may find this even more annoying. (and by "user" I mean me, of course) I think that this will also simply the unit tests, although I haven't really thought through that aspect. |
Sorry, I think I misread your first proposal. That would work. We could probably just use try/except in a single |
My feelng is, that it would be the best to skip inclusion of SPA until we have a pure python version (in case it is allowed to use the algorythm, but not the C implementation). If there is a matlab version, we could try to "translate" it. |
I got part way through translating the matlab code, it is definitely possible though because of the differences in matrix handling between matlab and python, it is not an easy task. However, I stopped because it doesn't make a lot of sense to re-write good, functional and validated C code in slower python. That being said, if licensing is the bottleneck, that that might be what we have to do. But, let me expand on the first proposal, with @wholmgren 's additions. Could we pull the spa code from the github repo, but maintain the compiling instructions in the setup.py, protected by a try/except clause. A user doing a pypi install will get the package with a non-functional spa module (and we will have to set up logging/debug appropriately). If they want to install spa, they will need to download the repo, insert their downloaded spa files, and re-run the setup. Would that make sense? |
For the sake of expediency, this is ok with me for the 0.1 release.
Beyond 0.1, I do think that we'd want to put the spa wrappers in their own package so that people don't have to go through this procedure every time they upgrade or freshly install pvlib. The people that bothered to do it for 0.1 will probably be ok doing it once more if we promise that it's the last time they need to. I'm imagining that we could have pvlib bug fix releases every 1-3 months, and feature releases every 3-6 months. That would make for a lot of unnecessary installation grief if the spa wrappers remain a part of pvlib. I agree that it doesn't make a lot of sense to rewrite the spa algorithm in python, licensing issues aside. However, if we do rewrite it, we could add an optional numba decorator to make it much faster. |
Not fully resolved, keeping this for context and shifting to the 0.2 milestone |
Just noticed this from PVLIB MATLAB's
So they've apparently run into the same problem. |
I think this is solved now, isn't it? So I would move it to the 0.1 milestone and close it. |
It appears to me that we may be in violation of NREL's SPA license
"The Software is being provided for internal, noncommercial purposes only and shall not be re-distributed."
A couple of potential problems:
The license goes on to say that you must contact them about using it in commercial applications.
Am I reading this wrong? @Calama-Consulting did you have a different interpretation of this when you added the SPA code to pvlib?
The text was updated successfully, but these errors were encountered: