-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add PVSystem pvsyst_celltemp methods #636
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've added the methods to implement pvsyst_celltemp in PVSystem and ModelChain and PVSystem test that was straightforward as expected. I didn't get a clear sense of a test for ModelChain's implementation. I took time to try to implement a test for the ModelChain method but found that the new "temp_model" possibilities introduced in pvsyst_celltemp don't work well with running ModelChain's defaults. I copied "test_modelchain.test_run_model_with_weather" and customized it for watching it run ModelChain.pvsyst_cell but ran into errors based on the provided pvsystem fixture. I made my own fixture which got complicated so I left it out of this PR and see if I could get some insight on this topic. |
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.
Not sure what happens if default racking model is supplied when instantiating PVSystem
because only "freestanding"
and "insulated"
are acceptable choices pvsyst_celltemp
, not racking_model='open_rack_cell_glassback'
Have you guys noticed that GitHub now alerts users when a commit is force pushed to a PR? |
* fix _delta_kt_prime_dirint end values * syntax, test fixes: * syntax and test fixes * more test fixes * more test fixes * and more test fixes * and still more test fixes * add comments, update whatsnew * delete space
…lib-python into pvsyst_celltemp_methods
The two failing checks seem to be from outside the changed code base from my PR. Is there anything else I need to do to get this accepted? Just making sure you're not waiting for me. Thx. |
I'm ready to merge this. @wholmgren ? |
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.
Please see https://pvlib-python.readthedocs.io/en/latest/pvsystem.html for context on the requested API changes.
…lib-python into pvsyst_celltemp_methods
Just FYI, the stickler-ci fail deals with code that I didn't contribute but I can change it if necessary. |
@wholmgren Travis failure is a numpy/py27 conflict of some sort. py27-min build is OK. AppVeyor failure is I don't know why the merged code from #638 shows up here as a change, it must be because it was merged into @CameronTStark's branch via a pull request from me, rather than from pvlib-python/master. OK to merge? |
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.
LGTM. Thanks for the changes @CameronTStark.
Let's figure out the numpy issues in another issue. Could be a blocker for 0.6.1.
pvsyst_celltemp
method toPVSystem
#633docs/sphinx/source/api.rst
for API changes.docs/sphinx/source/whatsnew
file for all changes.Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):