Skip to content

Make sure times are assigned with ModelChain.run_from_poa#1162

Merged
wholmgren merged 2 commits intopvlib:masterfrom
alorenzo175:assign-poa-times
Feb 5, 2021
Merged

Make sure times are assigned with ModelChain.run_from_poa#1162
wholmgren merged 2 commits intopvlib:masterfrom
alorenzo175:assign-poa-times

Conversation

@alorenzo175
Copy link
Contributor

@alorenzo175 alorenzo175 commented Feb 5, 2021 •
edited by wholmgren
Loading

  • Closes #xxxx
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst 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.

When running ModelChain.run_from_poa, ModelChain.times is not assigned so that the solar position results are incorrect and have a NaT index.

The issue was introduced in #1076 here

pvlib-python/pvlib/modelchain.py

Lines 1196 to 1203 in b0d29f5

def _prep_inputs_solar_pos(self, kwargs={}):
"""
Assign solar position
"""
self.results.solar_position = self.location.get_solarposition(
self.times, method=self.solar_position_method,
**kwargs)
return self
, so I'm not sure if any what's new update is needed.

@wholmgren
Copy link
Member

wholmgren commented Feb 5, 2021

looks like the same issue exists in ModelChain.run_model_from_effective_irradiance. times is not separable from weather, so maybe it should be called within _assign_weather.

@alorenzo175
Copy link
Contributor Author

alorenzo175 commented Feb 5, 2021

Yes it is missing there too, but I guess not a problem since it doesn't compute and assign solar position. So move it to _assign_weather?

@cwhanse
Copy link
Member

cwhanse commented Feb 5, 2021

@wholmgren it appears that the only use of ModelChain.times is as input to a solar position calculation here and here. I image those calls used weather.index before the multi-array capability. I'm not sure that ModelChain.times is consistent with the rest of ModelChain attributes, and if it persists is likely to get baked into user applications because it is convenient.

I'm OK patching this now so that pvlib behaves consistently. But down the road I think we'll want to remove ModelChain.times in favor of perhaps ModelChain.results.times?

alorenzo175 added a commit to alorenzo175/solarperformanceinsight that referenced this pull request Feb 5, 2021
@alorenzo175
update for pvlib 0.9.0a4
6499498 
a4 because pvlib/pvlib-python#1162 is required
along with a tbd issue for computing from effective irradiance

closes SolarPerformanceInsight#119 closes SolarPerformanceInsight#109

also adds real tests of the pvlib modelchain to catch errors there
@alorenzo175 alorenzo175 mentioned this pull request Feb 5, 2021
Merged
@wholmgren
Copy link
Member

wholmgren commented Feb 5, 2021

I agree. A year ago or so I wanted to remove ModelChain.times entirely because it no longer served any purpose, but refactorings since then have brought it back to life. ModelChain.weather should also be moved to the results.

Let's move it to _assign_weather as a short term fix.

@wholmgren wholmgren added this to the 0.9.0 milestone Feb 5, 2021
@wholmgren wholmgren merged commit b40df75 into pvlib:master Feb 5, 2021
@alorenzo175 alorenzo175 deleted the assign-poa-times branch February 5, 2021 20:25
@cwhanse
Copy link
Member

cwhanse commented Feb 5, 2021

I agree. A year ago or so I wanted to remove ModelChain.times entirely because it no longer served any purpose, but refactorings since then have brought it back to life. ModelChain.weather should also be moved to the results.

Worth opening an issue so we don't forget about this

@wholmgren wholmgren mentioned this pull request Feb 5, 2021
Closed
alorenzo175 added a commit to SolarPerformanceInsight/solarperformanceinsight that referenced this pull request Feb 8, 2021
@alorenzo175
update for pvlib 0.9.0a4 (#124)
5db53ab 
* update for pvlib 0.9.0a4

a4 because pvlib/pvlib-python#1162 is required
along with a tbd issue for computing from effective irradiance

closes #119 closes #109

also adds real tests of the pvlib modelchain to catch errors there

* mypy

* test more modelchain configurations

* fix when missing poa global or solar position for from_effective_irr

* make sure poa_global is NaN not None

* use pvlib 0.9.0a4

* comment update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wholmgren wholmgren wholmgren approved these changes

Assignees

No one assigned

Labels

api bug

Projects

None yet

Milestone

0.9.0

Development

Successfully merging this pull request may close these issues.

3 participants

@alorenzo175 @wholmgren @cwhanse