Implement irradiance.complete_irradiance with component sum equations

[kperrynrel]

Added the function component_sum_irradiance() to independently calculate the irradiance ghi, dhi, and dni fields from the modelchains module.

• Closes Add in component sum equations for GHI, DHI, and DNI into the pvlib.irradiance module. #1565
• 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.

[cwhanse]

Not a review, rather, thoughts for discussion:

• I have a preference for irradiance.complete_irradiance because that's the ModelChain method that has done a similar operation.
• Would the user experience be improved if the function returned a DataFrame with 'ghi', 'dni' and 'dhi' columns? That's closer to what the rest of pvlib would expect, and it would simplify the parameter names here which I suspect is the reason for the 'ghi_series', etc., input parameter names.

'clearsky_dni' should be described as optional.

[kperrynrel]

@cwhanse on the second point, modelchains definitely prefers the dataframe with the dni, ghi, and dhi fields, but I worry about the use of a dataframe not in the context of modelchains. I set it up as separate fields for situations where the dataframe has uniquely named ghi, dni, and dhi columns. Open to either way, but that's what motivated the initial choice to make the ghi, dni, and dhi fields as separate parameters.

[cwhanse]

Is there an aversion to having e.g. 'ghi' be both an input and output parameter name?

[kperrynrel]

I don't have an aversion to it if we change the name of the pvlib.irradiance.dni() function. I originally had the fields ghi, dhi, and dni as passed parameters, but it was erroring since we also have a dni() method (got confused between parameter and method). What do you think about renaming the method?

[kandersolar]

I think @cwhanse is suggesting keeping the input parameters as separate series but returning the outputs together in a single dataframe, which I agree is more consistent with other functions in pvlib.irradiance that return multiple irradiance variables. Sorry @kperrynrel I wasn't clear when we were talking before; I meant that dataframes are to be avoided as inputs to functions, but sometimes it makes sense to use them for outputs.

Something related to this if you're up for it: with this kind of function, we try to write it such that it will work equally well for scalar, numpy, and pandas inputs, and return outputs of similar type (scalar in -> scalar out, numpy in -> numpy out, pandas in -> pandas out). In this case for scalar inputs we'd return a dictionary instead of a dataframe. pvlib.irradiance.campbell_norman is a good example to follow here. One complication is that the dni function apparently doesn't follow this style and won't work as-is with scalars -- it might need a bit of rewrite to allow that if we want the new function to work with all input types. If not, ok with me to just assume pandas for now and do a follow-up PR later to add support for other types.

Regarding the dni name collision: I suggest naming the input parameter dni for consistency with the rest of pvlib and calling the dni function via pvlib.irradiance.dni(...) (this will require an import pvlib at the top of the file). It's not really ideal, but it avoids a breaking change and/or inconsistent parameter names.

[kperrynrel]

Ok, @kanderso-nrel that makes more sense. Thank you guys both for the suggestions @kanderso-nrel and @cwhanse ! I'll see what I can do to adapt the code to make it fit better with the pvlib framework.

[kandersolar]

Here's a partial review

[kandersolar]

Getting close! Sorry I'm so nitpicky about everything...

[kandersolar]

Hmm, I guess this is an artifact of switching to apparent_zenith. Maybe we shouldn't actually make that change, or if we do then it needs to be mentioned in the changelog. Let's keep this as "to discuss" also.

[cwhanse]

On technical merit, apparent_zenith would be better. But the practical difference is small and not worth annoying someone trying to reproduce archived results. I could go either way: change to apparent_zenith, or name the parameter solar_zenith and give a generic description.

[cwhanse]

Any other input on this change? We have two votes neither opposed but neither in strong favor. If that's the roll call, I'd say keep it as zenith.

[kandersolar]

@pvlib/pvlib-core, any thoughts regarding the apparent zenith question (#1567 (comment) and #1567 (comment))?

A few other minor things below, otherwise LGTM

[cwhanse]

Pending zenith vs. apparent_zenith, I'm approving.

[cwhanse]

On technical merit, apparent_zenith would be better. But the practical difference is small and not worth annoying someone trying to reproduce archived results. I could go either way: change to apparent_zenith, or name the parameter solar_zenith and give a generic description.

[cwhanse]

Any other input on this change? We have two votes neither opposed but neither in strong favor. If that's the roll call, I'd say keep it as zenith.

[wholmgren]

A few minor comments that I'd prefer to see addressed but won't object to ignoring.

Can we change the PR name (and thus the default commit) to something like "Implement irradiance.complete_irradiance with component sum equations"? The current title makes me think of the pvanalytics QC function.

[kperrynrel]

@wholmgren thank you for the review! I went ahead and worked on addressing all of your comments, and changed the PR name to the suggested name. Let me know if there's anything else I can do before merging!

[wholmgren]

thanks @kperrynrel! @kanderso-nrel can you merge if you're happy with it too?

[kandersolar]

Going back to zenith meant the associated ModelChain test changes needed to be undone as well, but LGTM now. Thanks @kperrynrel!