Skip to content

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

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 32 commits into from
Aug 19, 2022

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Jun 21, 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 (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Re-opens #1469.

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 marked this pull request as ready for review June 21, 2022 18:33
@cwhanse cwhanse added this to the 0.9.2 milestone Jun 22, 2022
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Some very minor comments, but LGTM. I think we've said in the past that there's no need to add new functionality to SingleAxisTracker, but I don't mind leaving it in seeing as you've already gone to the effort of adding it :)

@cwhanse cwhanse mentioned this pull request Jul 26, 2022
9 tasks
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 @cwhanse for the big lift.

[0., -0., 0., 0., 0.]]),
columns=['poa_global', 'poa_direct', 'poa_diffuse', 'poa_sky_diffuse',
'poa_ground_diffuse'],
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.

I think we need similar test for Array.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not seeing the gap to be filled by an Array test. PVSystem.get_irradiance is (mostly) a wrapper for Array.get_irradiance, so this PVSystem test confirms that data passes through to irradiance.get_total_irradiance and back.

Copy link
Member

Choose a reason for hiding this comment

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

That's true, but Array is an independent part of the API. So far we've been pretty good about testing that each layer of the API returns the correct values or make assertions that the correct parameters are passed through. A relatively easy approach would be to create a copy of test_PVSystem_multi_array_get_irradiance with values set for all of the optional method arguments.

I won't object if others think this is unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. It's just that we should construct a new solution due to changing the kwargs from default. test_PVSystem_multi_array_get_irradiance_multi_irrad confirms that PVSystem.get_irradiance and Array.get_irradiance return consistent values.

I note a small inconsistency has crept in: Array.get_irradiance defaults to the Hay-Davies model, whereas irradiance.get_total_irradiance defaults to the isotropic sky diffuse model.

@wholmgren
Copy link
Member

@cwhanse this PR has one line that fails stickler. any other reasons to hold off on merging it?

@cwhanse
Copy link
Member Author

cwhanse commented Aug 17, 2022

@wholmgren it's ready as far as I know. I fixed that line.

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.

@kanderso-nrel ready to merge?

@kandersolar
Copy link
Member

Reading back through #1387, several people advocated for raising an error if albedo was specified in both weather and as a property of the PVSystem/Array, but I don't think this code does that. Oversight, or intentional?

@cwhanse
Copy link
Member Author

cwhanse commented Aug 18, 2022

Reading back through #1387, several people advocated for raising an error if albedo was specified in both weather and as a property of the PVSystem/Array, but I don't think this code does that. Oversight, or intentional?

Oversight. Code will be emended soon.

@cwhanse
Copy link
Member Author

cwhanse commented Aug 18, 2022

On third thought, I think the current behavior (weather albedo is used when present, otherwise PVSystem.arrays albedo is used) is better than either raising a warning or error if albedo is provided in both weather and on PVSystem.arrays.

The default for PVSystem.Array is 'albedo'=None, which results in an assigned albedo (either from surface_type or 0.25). No Array is instantiated without an albedo. weather containing 'albedo' would then always generate a warning or error.

@cwhanse
Copy link
Member Author

cwhanse commented Aug 18, 2022

One improvement that could be made: when albedo is on ModelChain.system.arrays, it is not currently being transferred to ModelChain.results.albedo. Make that part of this PR, or a follow-on? Done in this PR.

@kandersolar kandersolar merged commit e1393f7 into pvlib:master Aug 19, 2022
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