Remove empty horizon component from irradiance.haydavies#2788
Open
cbcrespo wants to merge 3 commits into
Open
Conversation
AdamRJensen
reviewed
Jun 19, 2026
Member
|
@kandersolar This is technically a breaking change. Do we want a deprecation period (which will slow down the GSoC project a lot - probably not a good reason not to do it)? |
Member
I'd love one, but unfortunately we don't have a good mechanism for deprecating dict/dataframe keys (see #2530). If we go forward with this change, I think all we can do is just do it. Not related, but fyi: I need to review the discussion about not including empty components, since it's a change in direction and I don't remember the arguments for/against. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #xxxxTests addeddocs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor 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`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.This PR is part of my GSoC 2026 project. One of the goals is to add support for
return_componentsto all diffuse transposition models inpvlib, which currently is supported by some models (e.g.perez) but not others. This parameter allows the user to choose whether they want only the total diffuse irradiance (default), or are interested in the split between different diffuse components (sky diffuse, circumsolar, horizon brightening).haydaviesis one of the models which already supportsreturn_components. However, the model itself supports only sky diffuse and circumsolar components, not horizon brightening. Currently, the function returns a horizon brightening component filled with zeros. As suggested by @adriesse here, I've decided not to include empty components in new additions and so, for consistency, I've also modifiedhaydaviesto remove the horizon brightening component.In addition, I've changed the existing
OrderedDictto adict, as suggested by @kandersolar (see #1684).The docstrings and the relevant test were also modified accordingly.