-
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 |
wholmgren
left a comment
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?
| * 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
| n_workers_for_parallel_calcs: int, default 2 | ||
| 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
wholmgren
left a comment
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.rstfor API changes.docs/sphinx/source/whatsnewfile for all changes.Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):
pvfactorshad a major release with an updated package API: so need to update implementation inpvlib-python