Add JRC spectral factor correction

[RDaxini]

• Closes add JRC spectral factor model #2087; partially closes Google Summer of Code project summary: Implementing new spectral corrections in pvlib #2065 and Add new spectral factor models #1950
• I am familiar with the contributing guidelines
• Tests added
• 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.

Would like some advice on some questions raised in #2087 regarding citations and the airmass parameter.
Docs: https://pvlib-python--2088.org.readthedocs.build/en/2088/reference/generated/pvlib.spectrum.spectral_factor_jrc.html?highlight=jrc#pvlib.spectrum.spectral_factor_jrc

[echedey-ls]

Some comments down below. Feel free to apply suggestions with your own style.

[echedey-ls]

I'm a bit in favour of handling KeyError's and having custom error messages with the available keys, but I remember some comment somewhere that deemed that unnecessary. Just in case someone else wants to weight in, I'm not requesting changes.

[RDaxini]

Think I've made all the changes recommended so far from @echedey-ls @cwhanse. Let me know if there is anything else

[cwhanse]

Add an entry to whatsnew and LGTM

[echedey-ls]

I don't quite understand the derivation of the mismatch equation. Eq. (2) in the model includes the reference specific SC current.

$I_{sc}^*(k_c, AM) = I_{sc0}^* + k_1(e^{-k_c}-e^{-1}) + k_2(k_c-1) + k_3(AM-1.5)$ Eq. (2)

By definition of the mismatch:

$M = \frac{P}{P_{nom}} \approx \frac{I_{sc}^*}{I_{sc0}^*} = 1 + \frac{k_1(e^{-k_c}-e^{-1}) + k_2(k_c-1) + k_3(AM-1.5)}{I_{sc0}^*} $

I think the specific sc current denoted in the paper must be included as described above. Feel free to correct me if I'm wrong.

[RDaxini]

@echedey-ls I*_sc0 can be divided out of the constant terms k_1, k_2, k_3 so the equation is correct without the I*_sc0 denominator, but I need to update the table of coefficients for these two specific modules.

[echedey-ls]

That's good from my side, but a few thoughts:

• don't precalculate those new coefficients outside the code, try to preserve as much original numbers as the paper, specially to keep readability and traceability of the calculations
• specific Isc should be accessible from the outside the same way the coefficients are

[RDaxini]

@echedey-ls ah, I just saw your comment after working on the commit. I understand your point.
I think it's normal to present a model for the spectral mismatch with normalised coefficients, but I understand that's not exactly what is presented in this paper since they present a model for specific current rather than mismatch. I could retain the commit and update the docs to explain this more clearly, or reverse the commit and add in I*_sc0 for the modules. I am not sure what is best in this case--- @cwhanse @kandersolar @AdamRJensen any thoughts...?

[cwhanse]

I think the function should return the mismatch multiplier $M$ rather than the "specific short circuit current" so that it is similar to other spectral modifier functions.

I'm inclined to not ask for $I_{sc0,ref}$ as input. The $I_{sc0,ref}$ value in Table 2 of the paper looks like a result of fitting (evaluate the fit at the STC point), rather than a value taken from a datasheet. $I_{sc0}$ is intrinsically connected with $k_1, k_2, k_3$ since all four were determined simultaneously by regression. So it makes little sense (to me) to input a value and attempt some kind of scaling of the coefficients, then divide the result by $I_{sc0,ref}$ to get back to a modifier of 1 at the STC point.

The built-in parameters can be computed within the function, so that it is explicit how to connect the code back to the paper.

[RDaxini]

see last commit --- @cwhanse did I understand your suggestion correctly? include the division for built-in coefficients inside the function?

[echedey-ls]

That way of solving it is completely valid. An alternative would be to use a pandas dataframe to include all the coefficients, Isc* and only make the division when needed. E.g.:

coeffs = pd.DataFrame(
    index=["CdTe", "si"],
    columns=["Isc*", "k_1", "k_2", "k_3"],
    data=[
        [Isc, k1, k2, k3],  # cdte
        [Ics, k1, k2, k3],  # si
    ]
if use_internal_coeffs:
    eq_coeffs = coeffs.loc["CdTe"] / coeffs.loc["CdTe"]["Isc*"]
    # use k1 as eq.coeffs["k_1"], etc.

But as a wise man once said, if it works don't touch it. Unfortunately I don't find a relevant xkcd.

Jokes and suggestions aside, right now there is no way of passing in a custom $I_{sc}^*$ - and as @cwhanse noted it is also one of the fitted parameters, IMO it should be part of the API, if coefficients are also exposed.

Since it would mix different types of coefficients and their units, the API for a function like this must be well-thought. With the pandas approach above, for example, you could say these external coeffs could be a dict or pd.Series with keys ["Isc*", "k_1", "k_2", "k_3"]. That's what I find easier to expose, thou there are other alternatives. E.g.: adding parameters.

Feel free to come up with other approaches too.

[cwhanse]

see last commit --- @cwhanse did I understand your suggestion correctly? include the division for built-in coefficients inside the function?

Your changes are what I had in mind.

@echedey-ls I think we can define the coefficients parameter to be the :math:a_k values in the docstsring's equation, and not have to request :math:I_{sc0}. If so, we should add a Note that the coefficients are not the same as indicated by :math:k_k in the paper.

[RDaxini]

I have updated the notes section of the docs to explain that the built-in coefficients differ from those published in the original manuscript. Feel free to recommend changes to phrasing there.
I think that's everything from my side.

[echedey-ls]

LGTM, thou I would try to clarify the airmass type before merging. I'm +1 to email the authors.

[kandersolar]

One minor note, otherwise LGTM

[AdamRJensen]

Great job @RDaxini!

Many thanks to @echedey-ls, @cwhanse, and @kandersolar for some great discussions

[RDaxini]

Thanks all:)