Skip to content
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

Component sum functions #163

Merged
merged 53 commits into from
Nov 15, 2022
Merged

Conversation

kperrynrel
Copy link
Member

@kperrynrel kperrynrel commented Sep 21, 2022

Added logic for component sum equations for GHI, DHI, and DNI. Worked with @PetersonUOregon to build out the logic.

  • Closes Adding component sum function calculations to the irradiance module #157
  • Added tests to cover all new or modified code.
  • Clearly documented all new API functions with PEP257 and numpydoc compliant docstrings.
  • Added new API functions to docs/api.rst.
  • Non-API functions clearly documented with docstrings or comments as necessary.
  • Adds description and name entries in the appropriate "what's new" file
    in docs/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`).
  • 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.

Sorry, something went wrong.

@cwhanse
Copy link
Member

cwhanse commented Sep 22, 2022

Two questions for discussion:

  • would these functions (e.g., DNI = function of GHI, DHI) be better imported from pvlib-python?
  • how do we protect against the inevitable "this value looks wrong" usually because of division by cos(zenith) when zenith is close to 90 (or >90)?

I like the fill_nighttime kwarg.

@kperrynrel
Copy link
Member Author

kperrynrel commented Sep 22, 2022

@cwhanse on your first point let me hook things up to pvlib--I like the idea of outsourcing here to existing packages because it makes our lives easier. I need to stew on point 2, @PetersonUOregon may have some good insight.

@PetersonUOregon
Copy link

The computation of DNI from GHI, DHI, and SZA is always a bit tricky near the horizon as @cwhanse said.

As it currently is the function does have a variable szalimit that can be used to minimize some of this unwanted behavior. For example setting the szalimt to 85 instead of 90, would result in division of .09, (not zero).

The other option here, is that we impose a QC to the output value to make sure that DNI does not exceed a clear sky model. But I expect that this is beyond the scope of this function.

@kperrynrel kperrynrel added the enhancement New feature or request label Sep 27, 2022
@kperrynrel kperrynrel requested a review from kandersolar October 3, 2022 19:02
@kperrynrel
Copy link
Member Author

@kanderso-nrel could you take a look at this PR? I couldn't find a good way to integrate any of the pvlib component sum functions into these functions, but the code is simplified enough that I'm not sure any integration is necessary. I also added an example gallery example.

@kandersolar
Copy link
Member

I'll happily review this but I think we should make sure we're all on the same page about goals first.

would these functions (e.g., DNI = function of GHI, DHI) be better imported from pvlib-python?

I'm torn; "completing the irradiance triangle" could be viewed as a gap filling method more suited to pvanalytics or an irradiance prediction model more suited to pvlib. I think I'd lean towards improving the functionality that pvlib already has along these lines (#157 (comment)), which as @kperrynrel points out is either incomplete (only capable of producing DNI) or not intended to be accessed by the user. Maybe a good starting point is promoting the body of ModelChain._complete_irradiance into a new function pvlib.irradiance.complete_irradiance?

@cwhanse
Copy link
Member

cwhanse commented Oct 3, 2022

To me, the two-to-three component calculations feel like they belong in pvlib, but the fill_nighttime function belongs here. I support "promoting the body of ModelChain._complete_irradiance into a new function pvlib.irradiance.complete_irradiance"

The code is in pvlib.modelchain.ModelChain because that's where it was being used, not for any good design reason.

@kperrynrel
Copy link
Member Author

Thanks @kanderso-nrel and @cwhanse --what do you guys think about me making two component sum functions for DHI and GHI calculations in PVlib, then importing these functions here, and applying the nighttime filter?

@cwhanse
Copy link
Member

cwhanse commented Oct 3, 2022

Instead of adding new functions, I think we take the guts out of pvlib.modelchain.ModelChain._complete_irradiance and make a new function in pvlib.irradiance. I like that the current method handles all three cases.

@kperrynrel
Copy link
Member Author

kperrynrel commented Oct 3, 2022

@cwhanse and @kanderso-nrel just tagged a new issue in PVLib. I can work on it this week, and then once we go through a review process and PR in PVLib, we pick this back up? pvlib/pvlib-python#1565

@kandersolar
Copy link
Member

Now that pvlib/pvlib-python#1567 has landed, what's the next step here? If pvlib keeps to its new quarterly release schedule, the new pvlib.irradiance.complete_irradiance function won't be included in a pvlib release until December sometime. I see two ways forward:

  1. If we wait until the pvlib function is available to import, will we bump our minimum pvlib to 0.9.4 (currently 0.8.0), or maybe just require the newest pvlib for that one function?
  2. Alternatively, we could temporarily vendor the pvlib function here with a TODO to eventually switch to importing it from pvlib.

@cwhanse
Copy link
Member

cwhanse commented Nov 1, 2022

  1. Alternatively, we could temporarily vendor the pvlib function here with a TODO to eventually switch to importing it from pvlib.

I'm fine with this option.

@wholmgren
Copy link
Member

wholmgren commented Nov 1, 2022

Make a new pvlib release! git tag v0.9.4; git push upstream master then merge the conda forge PR. forget about the rest. Or make a prerelease and pull that for CI purposes. And I would say that as a sister project to pvlib, pvanalytics should have no qualms about maintaining a very aggressive minimum pvlib.

@kperrynrel
Copy link
Member Author

I definitely think @wholmgren 's suggestion is the easiest if we can just cut a new pre-release, but I also am not a pvlib maintainer so it's up to you guys on what you'd like to do! I'm going to get started on integrating the pvlib code into this PR today, and will just vendor the function over until we make a final decision.

kperrynrel and others added 7 commits November 14, 2022 09:17
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
@cwhanse
Copy link
Member

cwhanse commented Nov 14, 2022

@kperrynrel take a look at the function's doc page, the Parameters and Return sections aren't rendering right. I tried to fix it so I would understand what was wrong, but no luck. Reminder: you'll need to pull upstream since I was committing changes.

@kperrynrel
Copy link
Member Author

@cwhanse pulling your commits down now and seeing what I can do to debug. Thanks for taking a look

@kandersolar
Copy link
Member

Looks OK to me on first glance, maybe your browser is caching an older broken version and a hard refresh will fix it? The only rendering issue jumping out at me is the link to the pvlib docs; that's just due to using markdown link syntax instead of RST syntax.

@kperrynrel
Copy link
Member Author

kperrynrel commented Nov 14, 2022

image

Is this what you're seeing @cwhanse ? Mine also looks pretty screwy. @kanderso-nrel what is yours rendering as?

@kandersolar
Copy link
Member

I suspect the broken link syntax is to blame and the browser isn't finding enough characters it's willing to use to break the url across two lines. Maybe a difference between chrome and firefox? I see this on FF:

image

@cwhanse
Copy link
Member

cwhanse commented Nov 14, 2022

Looks OK to me now with latest commit.

@kperrynrel
Copy link
Member Author

Looks good to me as well @kanderso-nrel

@cwhanse cwhanse merged commit 340e564 into pvlib:master Nov 15, 2022
@kandersolar kandersolar added this to the v0.1.3 milestone Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding component sum function calculations to the irradiance module
5 participants