Skip to content

Accept albedo in weather input to ModelChain.run_model method #1469

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 16 commits into from
Jun 21, 2022

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Jun 9, 2022

  • Closes ModelChain should accept albedo in weather dataframe #1387
  • I am familiar with the contributing guidelines
  • Tests added
  • [ ] Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

Tests pvlib.irradiance and pvlib.clearsky functions with albedo as a Series.
Allows albedo to be specified as a column in weather for ModelChain methods.

@cwhanse cwhanse added this to the 0.9.2 milestone Jun 13, 2022
@cwhanse cwhanse marked this pull request as ready for review June 13, 2022 19:38
@cwhanse cwhanse changed the title WIP: accept albedo in weather, permit albedo to be a Series Accept albedo in weather input to ModelChain.run_model method Jun 13, 2022
@cwhanse
Copy link
Member Author

cwhanse commented Jun 13, 2022

Ready for review, I think. PVSystem methods and functions expect albedo on each Array. I have left albedo as a float on each Array because it makes little sense (to me) to associate a Series with an Array property. I suppose one could index albedo by time of day, or zenith angle, but not in this PR.

I've added code to ModelChain.prepare_inputs to move albedo from weather to the Arrays, and to raise an error if albedo is found both on weather and on the Arrays. It looks clunky but seems to work.

try:
albedo = kwargs.pop('albedo')
except KeyError:
albedo = None
Copy link
Member

Choose a reason for hiding this comment

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

Why not include albedo=None explicitly in the function signature instead of try/except and kwargs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not include albedo=None explicitly in the function signature instead of try/except and kwargs?

It seems odd to add albedo as a PVSystem method argument when the corresponding property PVSystem.albedo is deprecated. Or, I was flailing around trying to get the tests to work.

@@ -8,15 +8,17 @@ Deprecations

Enhancements
~~~~~~~~~~~~
* albedo can now be provided as a column in the `weather` DataFrame input to
:py:method:`pvlib.modelchain.ModelChain.run_model`. (:issue:`1387`, :pull:`1469`)
Copy link
Member

Choose a reason for hiding this comment

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

I suppose the PVSystem and Array changes should be noted too

mc = ModelChain(sapm_dc_snl_ac_system_Array, location)
# albedo on pvsystem but not in weather
weather = pd.DataFrame({'ghi': 1, 'dhi': 1, 'dni': 1, 'albedo': 0.5},
index=times)
Copy link
Member

Choose a reason for hiding this comment

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

FYI this comment doesn't match the test code

if albedo is None:
try:
albedo = kwargs.pop('albedo')
except KeyError:
Copy link
Member

Choose a reason for hiding this comment

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

I think it's not possible for kwargs to contain albedo here since it's already a named parameter to this function.

@cwhanse cwhanse merged commit 577f3b5 into pvlib:master Jun 21, 2022
@cwhanse
Copy link
Member Author

cwhanse commented Jun 21, 2022

WHOOPS! I merged this by mistake. I had another github repo open and hit merge in the wrong browser tab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ModelChain should accept albedo in weather dataframe
3 participants