-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update pvfactors to v1.0.1 #722
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
Hi @wholmgren , here is a first pass at updating pvfactors in pvlib. Please let me know if you have any questions |
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.
Thanks @anomam! Looks good to me except for a note about the API changes.
Any comments from people using bifacial models?
@@ -31,6 +31,8 @@ Enhancements | |||
* Add EPW data reader to :ref:`iotools`. (:issue:`591`) | |||
* Add PSM3 reader to :ref:`iotools`. (:issue:`592`) | |||
* Improve ModelChain inference method error text. (:issue:`621`) | |||
* Update :py:func:`~pvlib.bifacial.pvfactors_timeseries` and tests to use | |||
``pvfactors`` v1.0.1 (:issue:`709`) |
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 note the breaking API changes in the API section of this file.
pvlib/bifacial.py
Outdated
Number of workers to use in the case of parallel calculations. The | ||
default value of 'None' will lead to using a value equal to the number | ||
default value of '-1' will lead to using a value equal to the number |
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.
default is 2 or is -1?
pvlib/test/test_bifacial.py
Outdated
|
||
# Run calculations in parallel | ||
ipoa_front, ipoa_back, df_registries = pvfactors_timeseries( | ||
ipoa_front, ipoa_back = pvfactors_timeseries( |
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.
@anomam I suggest splitting the run_parallel_calculations=False|True
into two tests. Maybe something like this (including optional xfail
if you don't know the reason for windows failures).
from conftest import platform_is_windows
@pytest.mark.parametrize('run_parallel_calculations', [
False,
pytest.param(True, marks=pytest.mark.xfail(platform_is_windows)
])
def test_pvfactors_timeseries(run_parallel_calculations):
# modify test as needed for arg
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.
Thanks @anomam!
pvlib python pull request guidelines
Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.
You may submit a pull request with your code at any stage of completion.
The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:
docs/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):
pvfactors
had a major release with an updated package API: so need to update implementation inpvlib-python