Skip to content

Run ModelChain from POA irradiance #943

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 29 commits into from
Sep 5, 2020
Merged

Run ModelChain from POA irradiance #943

merged 29 commits into from
Sep 5, 2020

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Mar 27, 2020

  • Closes Using ModelChain with Plane of Array (POA) as weather input #536
  • 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.

Provides methods to run a ModelChain starting with broadband plane-of-array irradiance or effective irradiance, and with cell temperature or module temperature. Creates a number of helper functions for these two methods.

@mikofski
Copy link
Member

Does this close #536 ?

@CameronTStark CameronTStark added this to the 0.7.3 milestone Mar 27, 2020
@cwhanse
Copy link
Member Author

cwhanse commented Mar 27, 2020

Does this close #536 ?

Not for the use case of a broadband instrument in the plane of array (e.g. pyranometer). Yes for the use case of a plane-of-array reference cell (with similar spectrum and surface reflectance).

@mikofski
Copy link
Member

Not for the use case of a broadband instrument in the plane of array (e.g. pyranometer). Yes for the use case of a plane-of-array reference cell (with similar spectrum and surface reflectance).

For the case of POA measured by pyranometer should we recommend using GTI-DIRINT to back-estimate the DNI & DHI components before using ModelChain or should there be a model chain that calls GTI-DIRINT under the hood given pyranometer-POA input?

Either way, I believe the documentation for ModelChain should clarify the point you make regarding POA from reference cell vs. pyranometer.

@wholmgren
Copy link
Member

Thoughts on 1. the current PR's approach of adding e.g. run_model_from_poa methods vs. 2. subclassing ModelChain into e.g. ModelChainPOA and overriding run_model ?

@cwhanse
Copy link
Member Author

cwhanse commented Mar 30, 2020

Not for the use case of a broadband instrument in the plane of array (e.g. pyranometer). Yes for the use case of a plane-of-array reference cell (with similar spectrum and surface reflectance).

For the case of POA measured by pyranometer should we recommend using GTI-DIRINT to back-estimate the DNI & DHI components before using ModelChain or should there be a model chain that calls GTI-DIRINT under the hood given pyranometer-POA input?

I'm hesitant to build that POA to DNI etc. to POA components translation into the method. We can inform the user that the GTI-DIRINT function is available, but they may have other information (e.g. DHI from a nearby weather station) and can find their own path to POA components.

Either way, I believe the documentation for ModelChain should clarify the point you make regarding POA from reference cell vs. pyranometer.

Absolutely. I think the docstrings for these methods should have an Example section.

@cwhanse cwhanse mentioned this pull request Apr 23, 2020
@cwhanse cwhanse modified the milestones: 0.7.3, 0.8.0 May 19, 2020
@cwhanse cwhanse closed this Jun 22, 2020
@cwhanse cwhanse deleted the poa branch June 22, 2020 14:20
@cwhanse cwhanse restored the poa branch July 28, 2020 18:27
@cwhanse cwhanse reopened this Jul 28, 2020
@cwhanse cwhanse added the SPI DOE SETO Solar Performance Insight project label Jul 30, 2020
@wholmgren
Copy link
Member

Good points. ModelChain is really a mess.

The primary ModelChain entry points (run_model, prepare_inputs, and complete_irradiance) all expect a single DataFrame with all relevant information. These methods don't require any other time series information from ModelChain attributes. I think we should keep this design pattern. Some historical context may help: ModelChain.run_model originally included three keyword arguments: times, irradiance, and weather. Over several releases, we've reduced it to just weather. Let's not go in the opposite direction. Let's also not start to rely on users setting explicitly attributes such as weather - no other pvlib methods require this and it's generally not a pattern I want to encourage.

Keeping that single input DataFrame design pattern while allowing for the input of were previously intermediate modeling results certainly raises issues with how data is grouped into attributes and how those attributes are named. My proposed solution for that is to generalize the DataFrame argument name to something like data, and then assign parts of data to the relevant ModelChain attributes. I'm open to a broader rethink, but it's difficult to see how that could be done cleanly without breaking a lot of user code.

@cwhanse
Copy link
Member Author

cwhanse commented Jul 30, 2020

@wholmgren where would we retain the intermediate outputs such as effective_irradiance? As new columns in data and as ModelChain attributes? I'm not sure I see the value in adding columns to a dataframe that is just passed as input to other functions, maybe collect all the attributes into a data attribute which is the Dataframe? We would reserve weather for external data that is passed into ModelChain methods, separate from quantities generated by ModelChain methods.

weather would contain ghi, dni, dhi, temp_air, wind_speed, precipitable_water, relative_humidity.
ModelChain.data would be a dataframe with everything else?

@wholmgren
Copy link
Member

wholmgren commented Jul 30, 2020

Sorry for being unclear. I am not in favor of modifying the input DataFrame. I'll try to be more concrete:

  • run_model(weather) --> No changes. We could, for consistency with new methods, rename weather to data, but a lot of people call this argument by its name (run_model(weather=weather)) due to the history of this signature so changing the argument name is not necessarily a minor API change.

  • run_model_from_poa(data) --> ModelChain selects known weather data (air temperature, wind speed, ghi, dni, dhi, precipitable_water, relative_humidity, more?) from data, stores on self.weather. Selects required POA columns from data and saves on self.total_irrad. Compute effective irradiance, module temperature, power as normal.

  • run_model_from_effective_irradiance(data) --> ModelChain selects known weather data stores on self.weather. Extracts known poa data and stores on self.total_irrad. Selects required effective_irradiance from data, stores on self.effective_irradiance. Compute module temperature, power as normal.

We could also assign a self.data but it's not required and I'm not sure if it's a good or bad idea.

@cwhanse
Copy link
Member Author

cwhanse commented Aug 17, 2020

  • run_model_from_poa(data) --> ModelChain selects known weather data (air temperature, wind speed, ghi, dni, dhi, precipitable_water, relative_humidity, more?) from data, stores on self.weather. Selects required POA columns from data and saves on self.total_irrad. Compute effective irradiance, module temperature, power as normal.

Working on this again. "selects known weather data" implies that we hardcode a list of keys that will comprise the weather dataframe attribute. This strikes me as difficult to maintain; when a new function is added somewhere (e.g. spectral model) which requires a new weather column, we'll have to add a key to this this list.

What about weather always, and only, containing weather-related quantities external to pvlib functions? It is assigned as is to ModelChain.weather to preserve the input and to provide data for downstream functions. Then run_model_from_poa would have two arguments: data, from which poa_global etc. is pulled, and weather.

To follow the rule "don't modify input dataframes" we shouldn't be filling in temp_air and wind_speed with default values if they aren't found in weather. Not sure I see how to work around this yet, looking for feedback about weather first.

@wholmgren
Copy link
Member

"selects known weather data" implies that we hardcode a list of keys that will comprise the weather dataframe attribute. This strikes me as difficult to maintain; when a new function is added somewhere (e.g. spectral model) which requires a new weather column, we'll have to add a key to this this list.

This is already the case. For example, air pressure in solar position

try:
kwargs = _build_kwargs(['pressure', 'temp_air'], weather)
kwargs['temperature'] = kwargs.pop('temp_air')
except KeyError:
pass

precipitable water in the first solar correction

def first_solar_spectral_loss(self):
self.spectral_modifier = self.system.first_solar_spectral_loss(
self.weather['precipitable_water'],
self.airmass['airmass_absolute'])
return self

I don't see how two DataFrame inputs improves the situation. I think it makes it worse since now I have to worry about if I am putting the data in the right place too.

To follow the rule "don't modify input dataframes" we shouldn't be filling in temp_air and wind_speed with default values if they aren't found in weather. Not sure I see how to work around this yet, looking for feedback about weather first.

Correct. I think we can fix this with a weather.copy()

@cwhanse
Copy link
Member Author

cwhanse commented Sep 3, 2020

Test is unrelated. To do, perhaps in follow-on PRs:

  • docstrings for new ModelChain constants
  • implement fallback to effective irradiance for cell temperature models, if ModelChain.total_irrad['poa_global'] is absent

@cwhanse cwhanse changed the title WIP: run ModelChain from POA irradiance Run ModelChain from POA irradiance Sep 3, 2020
@wholmgren
Copy link
Member

@cwhanse can you make issues for the follow up work?

@cwhanse
Copy link
Member Author

cwhanse commented Sep 3, 2020

@srlightfoote FYI if you are still watching.

@slightfoote
Copy link

thanks all! I think this feature will help ease the burden for folks wanting to use pvlib to model expected performance characteristics of operational plants where pyranometer sensors mounted at plane of array are the norm.

@cwhanse
Copy link
Member Author

cwhanse commented Sep 4, 2020

thanks all! I think this feature will help ease the burden for folks wanting to use pvlib to model expected performance characteristics of operational plants where pyranometer sensors mounted at plane of array are the norm.

@slightfoote that's the motivating use case. You may be interested in one of our current projects to build a modeling service for O&M use cases. There's some information at solarperformanceinsight.github.io

@wholmgren
Copy link
Member

Thanks @cwhanse!

@wholmgren wholmgren merged commit 0271a3e into pvlib:master Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement SPI DOE SETO Solar Performance Insight project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using ModelChain with Plane of Array (POA) as weather input
5 participants