Add extraterrestrial and direct spectra of ASTM G173-03 standard spectrum (AM1.5)

[echedey-ls]

• Closes Add full ASTM G-173-03 tables #1963
• 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.

Adds the remaining extraterrestrial and direct spectra of the AM1.5 standard spectrum bundled with pvlib.

Currently, only get_am15g allows retrieving of the global spectra.

The proposed approach adds a new function pvlib.spectrum.get_ASTM_G173 which returns a pandas.DataFrame. API designed similarly to get_am15g, although numpy.interp has been used instead of the scipy counterpart since the latter is deprecated.

I've changed get_am15g to only be a wrapper of this new addition (so we can save the space by removing its data source file).

Feel free to point any suggestions, tips or discuss the solutions.

Docs

• https://pvlib-python--2039.org.readthedocs.build/en/2039/reference/generated/pvlib.spectrum.get_reference_spectrum.html
• https://pvlib-python--2039.org.readthedocs.build/en/2039/gallery/spectrum/plot_standard_ASTM_G173-03.html
• https://pvlib-python--2039.org.readthedocs.build/en/2039/reference/effects_on_pv_system_output/spectrum.html

[kandersolar]

This is looking great @echedey-ls! Thanks for making it easily expandable in the future :)

[echedey-ls]

Thanks for the in-depth review @kandersolar !! Ready for another one 👀

Should I open an issue with the deprecation, so it's tasked in a milestone?

Docs

• https://pvlib-python--2039.org.readthedocs.build/en/2039/reference/generated/pvlib.spectrum.get_reference_spectrum.html
• https://pvlib-python--2039.org.readthedocs.build/en/2039/gallery/spectrum/plot_standard_ASTM_G173-03.html
• https://pvlib-python--2039.org.readthedocs.build/en/2039/reference/effects_on_pv_system_output/spectrum.html

[markcampanelli]

@echedey-ls Even with the new function staying in the spectrum subpackage, I am still a strong proponent of using the function name and signature

pvlib.spectrum.get_spectra_collection(*, collection="ASTM G173-03" )

instead of

pvlib.spectrum.get_reference_spectrum(wavelengths=None, *, standard="ASTM G173-03")

The proposed alternative function name allows future extension to providing non-standards-derived spectra collections without another function name issue/change, and it also reflects that in general a dataframe of (plural) spectra is returned.

Removing the interpolation option keeps a separation of concerns between data provision and data transformation in the lowest level functions. That said, I realize that this creates the need to add an additional interpolation function to fully replace the existing functionality in the deprecated pvlib.spectrum.get_am15g, which already has the interpolation option. Note: the iam subpackage separates out the interpolation functionality here, so my suggestion could be viewed as bringing more consistency to the overall API.

This is an example usage pattern (with subsequent interpolation not shown):

ASTM_G173_03_SPECTRA = get_spectra_collection(collection="astm_g173_03")  # pandas.DataFrame
ASTM_G173_03_SPECTRA_EXTRA = ASTM_G173_03_SPECTRA["extraterrestrial"]  # pandas.Series
ASTM_G173_03_SPECTRA_GLOBAL = ASTM_G173_03_SPECTRA["global"]  # pandas.Series
ASTM_G173_03_SPECTRA_DIRECT = ASTM_G173_03_SPECTRA["direct"]  # pandas.Series

[kandersolar]

@markcampanelli can you give an example of a spectra collection this function could be expanded to include in the future? I'm having trouble thinking of anything that would make sense to provide alongside standards-derived spectra except for other standards-derived spectra.

If we're talking about particular datasets (rather than static reference spectra) like the spectra measured at SNL (Data Hub link) or the Spectral On Demand service, I think we should be talking about one or more separate functions, likely in iotools.

Note: the iam subpackage separates out the interpolation functionality here, so my suggestion could be viewed as bringing more consistency to the overall API.

In my view, pvlib.iam.interp is intended to be an IAM model akin to physical or martin_ruiz. It happens to perform calculations similar to what we're talking about here, but the motivation is rather different, I think. Its primary purpose isn't to "re-align" an input IAM profile to some set of convenient AOIs, so it's not really analogous to the spectra interpolation here. My two cents, anyway!

in general a dataframe of (plural) spectra is returned

Good point! How about get_reference_spectra?

[markcampanelli]

@kandersolar One case would be a collection of test-problem spectra, but I suppose those types of collections could/should live in a different data provider, be it a different method in pvlib.spectrum or in pvlib.iotools. Sticking to just reference spectra, get_reference_spectra sounds good to me.

I suppose I view pvlib.iam.interp as another function that does double duty (which I generally think adds complexity and potentially inefficiency in usage), as it both transforms a dataset into an IAM function and then computes that function on input angles. That said, I see your point about it's different role. Keeping the dual role here, then I think this signature would be best:

pvlib.spectrum.get_reference_spectra(*, standard="ASTM G173-03", wavelengths=None)

(I don't know pvlib's guidance here, but I prefer to make all passed function arguments require identification by keyword, by using * at the beginning of the signature.)

[echedey-ls]

Thanks @markcampanelli for your feedback, I will change the function name 😃

Regarding the proposed signature, although I prefer reading code with the parameter names, I think it's unnecessary for this function. With just two parameters, one that accepts numbers and other that accepts strings, I doubt we will have any trouble reading someone else's code.

[markcampanelli]

Regarding the proposed signature, although I prefer reading code with the parameter names, I think it's unnecessary for this function. With just two parameters, one that accepts numbers and other that accepts strings, I doubt we will have any trouble reading someone else's code.

@echedey-ls So, are you sure you want to include the * in your current function signature? What is it supposed to accomplish?

def get_reference_spectra(wavelengths=None, *, standard="ASTM G173-03")

[echedey-ls]

So, are you sure you want to include the * in your current function signature? What is it supposed to accomplish?

@markcampanelli sorry for not being clear enough. I'm usually akin to use keyword-only parameters, but I deem using them here unnecessary. One is a list of numbers that will rarely be provided as a list of magic numbers (i.e., it will in a reasonably-named variable) and the other, a mostly-self-explanatory string.

[markcampanelli]

So, are you sure you want to include the * in your current function signature? What is it supposed to accomplish?

@markcampanelli sorry for not being clear enough. I'm usually akin to use keyword-only parameters, but I deem using them here unnecessary. One is a list of numbers that will rarely be provided as a list of magic numbers (i.e., it will in a reasonably-named variable) and the other, a mostly-self-explanatory string.

@echedey-ls I think the disconnect here is that in my view the wavelengths and standard arguments are "equally optional, both with default values", and I typically view ordering requirements as a source of code complexity. (Here, the root complexity is due to the fact that the function is responsible for multiple tasks.) My understanding of your intent is that if one doesn't name the wavelengths argument, then it must come first, and whenever the standard argument is used, then it must be named.

To be entirely fair, my preference for named-only function arguments (with or without default values) can cause issues when injecting functions into, say, numpy functions that are not designed to handle kwargs. (In this case the complexity is recreated from the need for a wrapper function or some other workaround.) IMHO, Python is perhaps too flexible in the way one can construct function signatures: https://discuss.python.org/t/confusion-regarding-a-rule-in-the-zen-of-python/15927.

[echedey-ls]

That's a fair point.

Anyone wanna weigh in their opinion too? Rn I'm unsure of what to do.

[markcampanelli]

@echedey-ls I am not trying to request changes at this point, I'm just trying to make the reasons behind my inquiries more clear. It sounds like the maintainers are ok with this PR, and I do not intend to delay you further as it provides significant value :).

Having worked on several large Python projects in production, I have seen how small complexities turn into weird unforseen headaches later. I would recommend these commentaries:

• https://www.youtube.com/watch?v=rI8tNMsozo0
• https://grugbrain.dev/

Also, there are counter arguments where complexity is justified (it's by no means black and white), such as "the lego fallacy" (a pile of legos is not always helpful to dump on a user trying to solve a problem), and locality of behavior.

[kandersolar]

One minor test comment, otherwise LGTM!

Barring any requests for changes, I'll merge this PR next week.

[echedey-ls]

@markcampanelli my apologies for being so stubborn. I think I was confused (I don't remember exactly what came to my mind when I engaged in that conversation earlier). I've re-read that conversation and you are completely right, that keyword only asterisk does not serve any purpose. I've removed it. Thanks for bearing with me.

[markcampanelli]

@echedey-ls No need to for you to apologize or feel silly. Like I said, I think Python would have been better off just making all function/methods arguments keyword only. I find the resulting code considerably easier to read because it becomes self-documenting, and it’s harder to break an interface if/when it grows, inc. if a default value is added.

Going the other way, I appreciate you bearing with my raving on about complexity and concerns about API consistency 🙂.

[kandersolar]

Thanks @echedey-ls for the PR and @markcampanelli for the reviews!