Skip to content

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

Merged
merged 7 commits into from
May 15, 2019
Merged

Conversation

anomam
Copy link
Contributor

@anomam anomam commented May 14, 2019

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:

  • Closes pvfactors 1.0.0 breaks bifacial.pvfactors_timeseries #699
  • I am familiar with the contributing guidelines.
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
  • Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

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 in pvlib-python

@anomam
Copy link
Contributor Author

anomam commented May 14, 2019

Hi @wholmgren , here is a first pass at updating pvfactors in pvlib. Please let me know if you have any questions

Copy link
Member

@wholmgren wholmgren left a 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`)
Copy link
Member

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.

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
Copy link
Member

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?


# Run calculations in parallel
ipoa_front, ipoa_back, df_registries = pvfactors_timeseries(
ipoa_front, ipoa_back = pvfactors_timeseries(
Copy link
Member

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

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @anomam!

@wholmgren wholmgren added this to the 0.6.2 milestone May 15, 2019
@wholmgren wholmgren added the api label May 15, 2019
@wholmgren wholmgren merged commit 7b6cf7a into pvlib:master May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pvfactors 1.0.0 breaks bifacial.pvfactors_timeseries
3 participants